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

Fix parent / child assignment logic fix #31

Closed
wants to merge 2 commits into from
Closed

Conversation

kwokcb
Copy link
Owner

@kwokcb kwokcb commented Oct 20, 2022

Fix AcademySoftwareFoundation#1109

Change

The matching logic (string) was "reversed" for membership so that parent assignments would override child assignments. That is the logic should be to check if the path of the assignment is completely contained within the path to the geometry being tested.

e.g.

  • The assignment string "/a/b" is contained in path to the geometry: "/a/b/c" so the material can be assigned to "/a/b/c"
  • The assignment string "/a/b" is contained in path to the geometry: "/a/b" so the material can be assigned to "/a/b"
  • The assignment string "/a/b/c" is not contained in path to the geometry: "/a/b" so the child's material cannot be assigned to the parent geometry.

Desktop

parent_child_assignment2

Web

parent_child_assignment

Copy link

@emackey emackey left a comment

Choose a reason for hiding this comment

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

Confirmed that the desktop viewer fix works for my test case and the updated chess set.

I was unable to test the JavaScript build, sadly I do not have emscripten installed to build it correctly.

@@ -605,7 +605,7 @@ export class Material
// and assign to the associatged geometry. If there are no looks
// then the first material is found and assignment to all the
// geometry.
this._materials = [];
while (this._materials.length) { this._materials.pop(); }
Copy link

Choose a reason for hiding this comment

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

I think this can get a performance boost and cleaner code just by writing:

this._materials.length = 0;

@kwokcb
Copy link
Owner Author

kwokcb commented Oct 21, 2022

Official PR AcademySoftwareFoundation#1113
has been put up. Closing local PR.

@kwokcb kwokcb closed this Oct 21, 2022
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.

Material assignments to child objects
2 participants