Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

egg: Implement egg object type rebuilding from scene graph to egg file #1092

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

darktohka
Copy link
Contributor

@darktohka darktohka commented Jan 1, 2021

Issue description

When converting BAM files to EGG files, for example, all EGG object types are lost in conversion.
This is because the BAM structure simply does not store the egg object types. It only stores the transformations that it has on the scene graph, but all information regarding which object type was used to induce those transformations is lost.

Because of this, it is not convenient to, for example, convert a BAM file to an EGG file, and then to a Maya MB file. A lot of information would be lost (alpha modes, collide masks, DCS flags, etc...). Modelers without access to the source file then have to spend extra time trying to figure out what egg object type to apply to what node.

Solution description

What if we could rebuild our object types using the information that is converted from the scene graph to the egg structure?

This PR adds a new strategy that rebuilds all object types in the egg structure, to the extent possible using the egg object types defined in the PRC config pages (egg-object-type-*).

Now, for example, when using bam2egg, all object types that match a group are added automatically to the group. This ensures that any further actions (for example, egg2maya) keep the object types, without losing them during the bam2egg process.

Here's a practical example:

Let's say we have an egg object type, ground defined in our configuration as such:
egg-object-type-ground <Tag> cam { shground } <Scalar> bin { ground }

When bam2egg converts the scene graph (with a "cam" tag set to "shground", and a "ground" bin name set) to an egg structure, it adds the following egg attributes:

<Tag> cam {
    shground
}
<Scalar> draw-order { 0 }
<Scalar> bin { ground }

Without the PR, the object type is completely lost.
With the PR, bam2egg yields the following attributes:

<ObjectType> { ground }
<Tag> cam {
    shground
}
<Scalar> draw-order { 0 }
<Scalar> bin { ground }

Keeping both the expanded version of the object type, and the ObjectType itself. These can finally be propagated further to Maya model files. Only object types that are defined in the loaded PRC files can be rebuilt.

To this end, I've added:

  • Equality and inequality operators to EggTransform and EggSwitchCondition
  • Object type satisfaction methods to EggGroup and EggRenderMode. They are similar to equality methods, but they only check equality for attributes that are explicitly set on the object type.
  • An egg object type rebuilding algorithm (does not support recursive egg object types, none of the built-in egg object types are recursive currently)
  • A new config option: egg-rebuild-object-types, which is disabled by default. It has to be set to true in order to enable egg object type rebuilding when explicitly using the EggSaver interface.
  • A new command line option for something-to-egg utilities: -notypes, which disables egg object type rebuilding when converting scene graphs to egg files. Egg object type rebuilding is enabled by default when using the utilities.

Built-in egg object types have been moved out of the source tree and into the PRC files. Two deprecated flags have been removed: trigger_sphere and eye_trigger, which have been previously defined as (and used as) trigger-sphere and eye-trigger in the PRC files.

Checklist

I have done my best to ensure that…

  • …I have familiarized myself with the CONTRIBUTING.md file
  • …this change follows the coding style and design patterns of the codebase
  • …I own the intellectual property rights to this code
  • …the intent of this change is clearly explained
  • …existing uses of the Panda3D API are not broken
  • …the changed code is adequately covered by the test suite, where possible.

@rdb
Copy link
Member

rdb commented Jan 1, 2021

Thanks for the PR!

First thoughts: code looks good, but… Wouldn't it be better to have the object types retained somehow when converting from egg to bam, rather than essentially try to guess what the original object type was? It seems a little fragile, at first blush: what if someone defines an object type that defines something very common, or matches multiple object types?

I'm also generally a little reluctant to add code to the bam-to-egg pipeline given that its maintenance cost is already unjustifiably high. If the maintenance overhead of this change is small and there is a real need for this feature, I will accept it, but there needs to be solid unit test coverage.

In the future it would be great to have a feature request / discussion issue before starting work on a big change like this.

@darktohka
Copy link
Contributor Author

darktohka commented Jan 1, 2021

Thank you for your thoughts rdb!

Indeed, the best option would be to have the object types retained in the BAM. This would add a little bit of overhead (egg object type information is not something that would be necessary in order to create the scene graph from the BAM), but would make things way more convenient.

But this PR is kind of clean up for past mistakes. Some projects have old models that have been added over 5-6 years ago by old contributors who have forgotten to upload the source models. Sometimes modelers have to fix those old models, and that is only possible by converting them back to egg, and then to Maya. Since we haven't considered saving object types to BAM yet, adding object types to the BAM at this point would only prevent this problem from happening in the future, but would not, by itself, allow for these old models to be salvaged.

I agree that we should save egg object types to BAM during the egg2bam pipeline. We could either save them as tags (which would not require a bam version change), or as their own field (would require a bam version change, but it's the better solution IMO).

Defining an overly-generic object type is a valid problem: this happens for "ghost" within the builtin object types. "ghost" is set to <Scalar> collide-mask { 0 }, so it pretty much appears on almost all nodes. Still thinking on a potential solution for that, as collide-mask is 0x00 by default... (does this object type even have any effect, then?)

If something matches multiple object types, that's no problem: all object types that match will be added. This isn't wrong by itself, because the behavior of the model will not change: but might introduce clutter.

Maintenance cost is fortunately not high for this feature: the only details a maintainer would need to pay attention to are located in the same file that they would be editing otherwise. For example, if somebody were to add a new Egg flag, or a new <Scalar> option, they'd be modifying EggGroup already, so it'd be hard for them to miss this method (and easy to add a check for their own feature).

I'm still thinking on potential ways to test this behavior. If I can find a way to create nodes within Python, set their attributes from Python code and then test for object types after saving the Egg, I will update the PR with some test cases.

My suggestion would be following:

  • After test cases are ready and working, focus on adding ObjectType serialization to BAM
  • Increment BAM minor version
  • When running bam2egg, object type rebuilding would only commence if our BAM is older than the ObjectType serialization changes.

Until ObjectType serialization is not added, we could change the -notypes flag to a -types flag instead, which would enable object type rebuilding, making this feature disabled by default.

As far as I am aware, this change would be super beneficial for many. (I've been asked to work on this, personally.)

@rdb
Copy link
Member

rdb commented Feb 7, 2023

I'm conflicted about this, and still on the side of "negatively inclined". This is an attempt to salvage data from what was intended to be a lossy conversion, reconstructing lost data based on guesses. You could end up with an egg file that has object types that the original never had, or with the wrong object type if you have overlapping object types.

I also don't like the idea of adding object types (which really are an egg-ism) to the .bam syntax. They would have no function in .bam, other than roundtrippability which already runs counter to the design of the egg pipeline. If you want to retain this information then just add a user-defined tag to your object type. But seeing as this wouldn't even address your use case, we'd be adding something without a use case.

We already make no guarantees about bam2egg so maybe if it's really considered generally useful we could end up including this but it would have to be disabled by default and with the affected "supported paths" covered by unit tests. I do feel like it might be better as a separate tool. I'll let others weigh in on this.

@rdb rdb added the discussion Issues opened primarilly to start a discussion label Aug 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issues opened primarilly to start a discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants