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

[ABC-312] Fix Face scope constant attributes #569

Merged
merged 33 commits into from
Dec 7, 2023

Conversation

SanaJal
Copy link
Contributor

@SanaJal SanaJal commented Nov 1, 2023

Purpose of this PR

Ticket/Jira #:ABC-312

When exporting the alembic with face scope, the custom attributes aren't processed as intended. This PR addresses this issue by introducing a method that uses the face count to produce an index array for the attributes.

Testing

Functional Testing status:

  • Manual testing :

    • Open a Unity project with the Alembic package installed
    • Import attributeScopeCubes.unitypackage package into the project
    • Instantiate the prefab attributeScopeCubes into a scene

Notice how the "cube_face" is now colored, each face with a color (this is the same result obtained when importing the cubes in Houdini).

  • Tests added :

new MeshSchema test class added :
- VertexRgb_AreProcessedCorrectlyForScope
- VertexRgba_AreProcessedCorrectlyForScope
- VertexUV_AreProcessedCorrectlyForScope
- VertexNormals_AreProcessedCorrectlyForScope

along with the missing data to test rgba, uv and normals.

rgb rgba uv
Normals

Performance Testing status: N/A

Overall Product Risks

Complexity:

Low

Halo Effect:

Minimal

Documentation & UX Writing

User facing text to review Details
User interface N
Public API docs N
Changelog Y
Documentation halo effect Jira link
User manual
Other docs

Additional information

Note to reviewers:

Reminder:

  • Add entry in CHANGELOG.md

@cguer cguer self-requested a review November 1, 2023 19:25
@SanaJal SanaJal marked this pull request as ready for review November 1, 2023 19:36
SanaJal and others added 3 commits November 2, 2023 11:51
Co-authored-by: thomas-tu <45040865+thomas-tu@users.noreply.github.com>
Co-authored-by: thomas-tu <45040865+thomas-tu@users.noreply.github.com>
Copy link
Contributor

@cguer cguer left a comment

Choose a reason for hiding this comment

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

I was randomly trying to generate more test data and stumbled onto one possible issue.

In Houdini I created a mesh with 4 triangles and a Cd attribute per triangle:

  • Triangle 0, Cd value = [1, 0, 0, 0]
  • Triangle 1, Cd value = [0, 1, 0, 0]
  • Triangle 2, Cd value = [0, 0, 1, 0]
  • Triangle 3, Cd value = [0, 0, 0, 1]

This is what it looks like in Houdini:
image

If Import the alembic in Unity and apply the Alembic/Overlay material this is what I see:
image

To be completely honest, I don't really know what I'm doing 😅, but I feel like the colors shouldn't appear to blend like this. I would assume to the same thing as in Houdini


I've attached the alembic file and the Houdini scene used to generate it in case it's useful (it's a .zip because Git doesn't want .hip/.abc files...)

plane_4tris_facecolor.zip

@SanaJal
Copy link
Contributor Author

SanaJal commented Nov 2, 2023

When trying to make the function more elegant, I hardcoded the refiner.count to 4 .. and forgot it like that ! I changed it now it gives me this :
image

it s the same colors but not the same positions of colors 🤔 ..

EDIT : WORKING AS EXPECTED
image

Copy link
Contributor

@cguer cguer left a comment

Choose a reason for hiding this comment

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

Found one more problematic case 🙈. The facecolor appears incorrectly "blended" if a mesh contains polygons with varying numbers of sides (ex: if a mesh contains a three-sided polygon and a four-sided polygon).

The first polygon appears correct and the others are "blended":

Houdini Unity
facecolor_Houdini facecolor_Unity

I've attached four files that can be used for testing facecolor_samples.zip :

  • plane_triQuad_facecolor_1_24fps.abc = 1 three-sided polygon, 1 four-sided polygon
  • plane_triNgon_facecolor_1_24fps.abc = 1 three-sided polygon, 1 five-sided polygon
  • plane_quadNgon_facecolor_1_24fps.abc = 1 four-sided polygon, 1 five-sided polygon
  • plane_ngonTriQuad_facecolor_1_24fps.abc = 1 three-sided polygon, 1 four-sided polygon, 1 five-sided polygon

I believe that we/Unity create a sub-mesh for every type of polygons. For example: if an alembic mesh contains both triangles and quads, all triangles will be grouped in a submesh and all quads will be grouped in another sub-mesh.

I'm not sure what the behavior is for polygons with more than 4 sides (ie: are they grouped by polydon-side-count or all together in a single sub-mesh)

I'm really not confident about the quality of this information though. I feel like someone reliable said this to me but it's in a very dusty corner of my memory, haha. I'm including it just in case helps for debugging

@SanaJal
Copy link
Contributor Author

SanaJal commented Nov 6, 2023

Good catch ! I reverted the changes in the getAttributesIndices method and it is working now !

image

@cguer cguer self-requested a review November 7, 2023 13:47
Copy link
Contributor

@cguer cguer left a comment

Choose a reason for hiding this comment

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

QA approved 👍!

I'm a bit limited in testing and knowledge around this area but every case I could think of seems to work now using the Color/Cd attribute + Alembic/Overlay material 🎉

Tested on:

  • 2023.2.0b15 (Windows only)

@SanaJal SanaJal requested a review from a team as a code owner November 9, 2023 18:35
@unity-cla-assistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


noreply@unity3d.com seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

com.unity.formats.alembic/CHANGELOG.md Outdated Show resolved Hide resolved
com.unity.formats.alembic/CHANGELOG.md Outdated Show resolved Hide resolved
Source/abci/Importer/aiMeshSchema.h Outdated Show resolved Hide resolved
SanaJal and others added 3 commits November 15, 2023 15:35
Copy link
Contributor

@thomas-tu thomas-tu left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

Choose a reason for hiding this comment

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

dumb question: Why is there actual code in this header file?

Copy link
Contributor

@arejayelle arejayelle left a comment

Choose a reason for hiding this comment

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

I ran it and it seems to work. The code logic follows from what I can tell. Just some wording stuff.

com.unity.formats.alembic/CHANGELOG.md Outdated Show resolved Hide resolved
[TestCase("cube_face")]
[TestCase("cube_point")]
[TestCase("cube_vertex")]
public void VertexRgb_AreProcessedCorrectlyForScope(string scope)
Copy link
Contributor

Choose a reason for hiding this comment

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

naming of the test functions could be clearer

Suggested change
public void VertexRgb_AreProcessedCorrectlyForScope(string scope)
public void Import_InScope_VertexRgb_Colors_AreAsExpected(string scope)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True ! changed here f30b011

SanaJal and others added 2 commits December 6, 2023 11:53
Co-authored-by: Jun Loke <jun.loke@unity3d.com>
@SanaJal SanaJal merged commit 6385bb5 into main Dec 7, 2023
33 of 34 checks passed
@SanaJal SanaJal deleted the ABC312-Fix-face-constant-attributes branch December 7, 2023 17:18
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