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

EXCEPTION REQUEST: Adds the ability to generate UVs automatically during mesh import #15675

Conversation

nick-l-o3de
Copy link
Contributor

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

Note to reviewers, this PR is already in 'development branch' but this is a PR to get it into the Stabilization branch as an Exception.
I will be adding the exception checklist below. Please see the original PR for a full brief including pictures of the difference it makes.

ORIGINAL PR from Development

#15530 - solves #12795 issue.
Without this PR merged, the robotics stuff in o3de-extras will seem incomplete for the release as all models will appear as featureless gray shadeless objects.

The new modifier is implemented in the same UX as the existing modifiers.
https://user-images.githubusercontent.com/70027408/229643430-e28c85ae-4e56-4be2-872c-619650ca2220.png

This PR is a cherry-pick of that squash submission.

Original PR text

  • 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

Exception Checklist

  • 1. Owning SIG requests the exception, by a Sig Representative (Chair/co-chair) adding the PR to the exception requests queue board.
  • 2. (SIG-Content) Owning SIG comments on the PR explaining impact if this code does not go into stabilization.
  • 3. Unrelated SIG representative (chair/cochair) approves the exception, adding a comment indicating approval and the SIG they represent.
  • 4. UX SIG representative (chair/cochair) approves the exception, adding a comment indicating approval and the SIG they represent.
  • 5. Testing SIG representative (chair/cochair) approves the exception, adding a comment indicating approval and the SIG they represent.
    (Must occur at some point before SIG-Release approval) Code has been approved for merging by maintainers
  • 6. SIG-Release Release Manager or co-Release Manager approves the exception, adding a comment indicating approval and the SIG they represent.
  • 7. If all approvals are accepted, then the exception PR may be merged
  • 8. Merged PR is verified successful via testing. If Merged PR verification testing failed, consult reps from step 3-5 and discussion occurs on fixing the failure vs backing out change
  • 9. Exception is complete

…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 nick-l-o3de marked this pull request as ready for review April 11, 2023 19:54
@nick-l-o3de nick-l-o3de requested review from a team as code owners April 11, 2023 19:54
@nick-l-o3de
Copy link
Contributor Author

nick-l-o3de commented Apr 11, 2023

In case it helps, if this is NOT merged, the import pipeline for assets specifically missing UVs (which are fairly common in the world of robotics/simulation) will come in as featureless unshaded gray blobs until the user trying to import them actually pulls them into a CAD package and generates UVs. This is particularly problematic since most of those users are not CAD experts and the files usually lack the original CAD source.

As for any risk introduced, this change introduces an option that is not enabled or added to any scenes by default, and even if added (by hand, by a human), it will not overwrite existing content unless a user explicitly checks a box on each and every instance they want it to happen on, individually.

@lemonade-dm
Copy link
Contributor

I am approving the exception as an unrelated SIG representative from SIG-Core as the need for mesh UV in the robotics models would really help with visually distinguishing the content.

@nick-l-o3de
Copy link
Contributor Author

nick-l-o3de commented Apr 11, 2023

To perform a manual test:
Unzip monkey_with_no_uvs.fbx to any assets folder (for example, AutomatedTesting/Assets)
monkey_with_no_uvs.zip
view in O3DE editor. notice it has no rendering and spews warning about missing UV sets. It renders as a featureless shadeless gray pixel field.

Right click the FBX Asset in asset browser -> "Edit Settings", "Add Modifier" -> UVs --> Save.

See how it now works just fine in the viewport and no longer spews warnings.

Note that only things you explicitly add this modifier to are affected in any way, and even adding the modifier will not affect anything that already has UVs.

@gadams3
Copy link
Contributor

gadams3 commented Apr 12, 2023

Missed that this was an exception request but I previously reviewed the changes.

@AMZN-Dk
Copy link

AMZN-Dk commented Apr 12, 2023

Approving this exception from SIG Testing, reviewed the changes and this looks like an impactful change that we should take.

@lmbr-pip lmbr-pip added the feature/scene-pipeline Tickets related to the scene pipeline label Apr 13, 2023
@lsemp3d
Copy link
Contributor

lsemp3d commented Apr 13, 2023

As chair of SIG-Content I approve this request, this improves quality when no UVs are present, it improves workflows & quality when it relates to bringing in assets into O3DE

@rainbj
Copy link

rainbj commented Apr 13, 2023

Spoke with Nicholas and talked about the UX use cases. No concerns at this point about UX pipeline

SIG-UI-UX approved.

@rainbj rainbj added the status/ux-approved The UX SIG has approved the UI/UX design for this feature. label Apr 13, 2023
Copy link
Contributor

@kberg0 kberg0 left a comment

Choose a reason for hiding this comment

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

I've read through the change and for the most part it looks really clean. I'll also approve as an unrelated sig representative, however I would really like to see a deeper review done by the asset team, as well as a plan for testing once merged, since this is going in without any associated unit tests and after the new feature cutoff.

@vincent6767
Copy link

vincent6767 commented Apr 14, 2023

As a SIG Co-chair, I approve this exception request.

@nick-l-o3de
Copy link
Contributor Author

@vincent6767 is the co-chair for sig-release. This means this ticket has met the necessary approvals to merge.

@nick-l-o3de nick-l-o3de merged commit a65fc4e into o3de:stabilization/2305 Apr 17, 2023
@nick-l-o3de nick-l-o3de deleted the exception_merge_uv_generation branch April 17, 2023 15:11
@tonybalandiuk
Copy link

Has this been tested post-merge @lawsonamzn ?

@nick-l-o3de
Copy link
Contributor Author

Yes, it works the same as it worked before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/scene-pipeline Tickets related to the scene pipeline status/ux-approved The UX SIG has approved the UI/UX design for this feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.