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

Adds the ability to generate UVs automatically during mesh import #15530

Merged

Conversation

nick-l-o3de
Copy link
Contributor

@nick-l-o3de nick-l-o3de commented Apr 3, 2023

What does this PR do?

This adds a new modifier to the available list of modifiers you can add to a mesh during its import in its import settings.

Pictures:
Before:
no_uvs

Add a modifier in the import settings
add_uvs

And done
after

The new modifier lets you select how to generate UVs for a mesh. Currently, only 1 generator is supported (Spherical Positional projection) but all the hard work here is done to support additional UV-unwrappers in the future by following the pattern (ie, adding to the case statement and adding to the enum).

#12795 is addressed partially by this.

The old behavior was that meshes that had no UV data had to have UVs added to them in order to render with the current set of PBR shaders. The only recourse was to go into something like blender and add UVs and re-export. However, in many situations like for example robotics or simulation, much of the input data do not have UVs and are meant for simple color or shape representation, or procedural texturing.

Spherical unwrapping isn't going to be as good as one of the more advanced unwrappers, but for getting some shading on meshes that really don't care about their material, its a start.

This adds a new modifier to the available modifiers for when you ingest meshes, just like the already existing "generate tangents" rule, you can now add a "generate UVs" rule. It follows that pattern, so doesn't really invent anything new.

If no modifier is present (all existing data), no changes are made.
If the modifier is present, the default is to use spherical UVs. The default can be changed in the regset file.

The change to the scenesettings file is as simple as adding

 {
                        "$type": "UVsRule"
}

to the list of rules applicable to the model. With optional "replaceExisting"

How was this PR tested?

Testing it manually using meshes that have no UV data and meshes that previously existed. The defaults are configured in such a way to have no effect whatsoever to existing data. Someone has to go and actually add these rules to the import settings for individual meshes to get the benefit. (Basically following in the footsteps of the tangent generation system).

@nick-l-o3de nick-l-o3de marked this pull request as ready for review April 4, 2023 14:57
@nick-l-o3de nick-l-o3de requested review from a team as code owners April 4, 2023 14:57
@nick-l-o3de
Copy link
Contributor Author

Consider turning off whitespace compare, as one of the files was ancient tab format and needed converting.

@nick-l-o3de nick-l-o3de requested a review from a team April 4, 2023 15:42
This adds a new modifier to the available list of modifiers you can
add to a mesh during its import in its import settings.

The new modifier lets you select how to generate UVs for a mesh.
Currently, only 1 generator is supported (Spherical Positional
projection) but all the hard work here is done to support additional
unwrappers in the future by following the pattern.

Signed-off-by: Nicholas Lawson <70027408+lawsonamzn@users.noreply.github.com>
Signed-off-by: Nicholas Lawson <70027408+lawsonamzn@users.noreply.github.com>
…hub.com>

Fixes from code review.

Signed-off-by: Nicholas Lawson <70027408+lawsonamzn@users.noreply.github.com>
@nick-l-o3de nick-l-o3de force-pushed the generate_uvs_for_meshes branch from 56cb390 to 3b30b28 Compare April 4, 2023 17:31
@nick-l-o3de
Copy link
Contributor Author

Force pushed to get it to latest dev and also refactored it so that the components are hidden in CPP files. The headers now only declare enough forwards to spawn the component in the module and the classes they use to communicate with each other, such as the Context classes.

Copy link
Contributor

@gadams3 gadams3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. Like you said, this should also make it easy to add other texture coordinate generators. Not sure how the spherical wrapping would look on something flat but it is clear how to add other methods that use position or normals to project onto the nearest plane.

Copy link
Contributor

@AMZN-stankowi AMZN-stankowi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be really helpful to have some automated tests around this new rule.

@nick-l-o3de
Copy link
Contributor Author

nick-l-o3de commented Apr 5, 2023

It would be really helpful to have some automated tests around this new rule.

I was following the pattern for tangent gen. Are there patterns to follow for tests of these generator components? I'll do a search and see if I can find them. If none exist, I'll see if its easy to add. OTOH, I don't have a huge amount of bandwidth to spend on this at the moment, else I'd have done something a lot more elaborate than just sphere map :)

Maybe I can leverage those existing pipeline tests and include an asset that has no UVs or something...

@AMZN-stankowi
Copy link
Contributor

I was following the pattern for tangent gen. Are there patterns to follow for tests of these generator components? I'll do a search and see if I can find them. If none exist, I'll see if its easy to add. OTOH, I don't have a huge amount of bandwidth to spend on this at the moment, else I'd have done something a lot more elaborate than just sphere map :)

I think the Python tests that process FBX files and compare the output to a last known good are probably enough for here: https://github.com/o3de/o3de/tree/development/AutomatedTesting/Gem/PythonTests/assetpipeline/scene_tests

Functionally these tests end up comparing the output to a last known good. I'll add some additional details on the tests and the strategy around them in the other pull request.

@nick-l-o3de
Copy link
Contributor Author

nick-l-o3de commented Apr 6, 2023

I'm actually not sure how useful these fbx_tests are. They don't run automatically, and largely seem like they cause a debug export of the scene data and cause it to be compared with expected scene data? That seems like I'd be just setting maintainers up for a huge headache anytime anyone changed anything about scenes.

I'm going to see if there's some other way to test that this basic functionality works. (STarting with "whats the most likely failure points for this for future maintainers + what would be the worst possible kinds of failures to occur"). I'm thinking "UV rule present, but no UV data is emitted" + "UV data is already present, 'do not overwrite' is set, and it overwrites anyway" or the like.

@nick-l-o3de
Copy link
Contributor Author

nick-l-o3de commented Apr 7, 2023

FWIW, I tried to fix and update those tests, (they were failing), only to find out that they're essentially useless.

  • Brittle
  • Respond to any change by failing, even benign changes in the data.
  • Respond to any additional product asset by failing
  • considers mutations of floating point numbers or hashes as warnings, not errors or asserts so the tests dont even fail when this happens and this is what you'd want to check in the case of UVs being overwritten.
  • currently full of failures...
  • does not run automatically (sandboxed) so nobody runs them.

I tried adding tests, but the framework is not useful right now.

Signed-off-by: Nicholas Lawson <70027408+lawsonamzn@users.noreply.github.com>
@nick-l-o3de nick-l-o3de merged commit 6c25f9f into o3de:development Apr 7, 2023
@nick-l-o3de nick-l-o3de deleted the generate_uvs_for_meshes branch April 7, 2023 20:19
nick-l-o3de added a commit to aws-lumberyard-dev/o3de that referenced this pull request Apr 11, 2023
…de#15530)

* Adds the ability to generate UVs automatically during mesh import

This adds a new modifier to the available list of modifiers you can
add to a mesh during its import in its import settings.

The new modifier lets you select how to generate UVs for a mesh.
Currently, only 1 generator is supported (Spherical Positional
projection) but all the hard work here is done to support additional
unwrappers in the future by following the pattern.

This ability is crucial for workflows to do with importing robotic
meshes as they often lack UVs, and the path to otherwise get something
reasonable to render is painful

Signed-off-by: Nicholas Lawson <70027408+lawsonamzn@users.noreply.github.com>
nick-l-o3de added a commit that referenced this pull request Apr 17, 2023
…5530) (#15675)

* Adds the ability to generate UVs automatically during mesh import

This adds a new modifier to the available list of modifiers you can
add to a mesh during its import in its import settings.

The new modifier lets you select how to generate UVs for a mesh.
Currently, only 1 generator is supported (Spherical Positional
projection) but all the hard work here is done to support additional
unwrappers in the future by following the pattern.

This ability is crucial for workflows to do with importing robotic
meshes as they often lack UVs, and the path to otherwise get something
reasonable to render is painful

Signed-off-by: Nicholas Lawson <70027408+lawsonamzn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants