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

Instancer : Fix handling of unindexed primvars in RootPerVertex mode #5554

Merged

Conversation

danieldresser-ie
Copy link
Contributor

As discussed, we actually had a test noting this incorrect behaviour, but it wasn't enabled.

I actually don't have a good feeling about this code ... trying to put a finger on it, I think there are a few factors:

  • we need to handle duplicate prototype names in this mode ... but do we not need to handle duplicate prototypes correctly in any other mode? I guess it's fairly reasonable to assume that an indexed primvar doesn't have too indices with the same value ( though I don't think anything would really prevent this ). There's nothing to prevent a user from entering duplicates if they manually enter a prototypeRootsList ... I guess it's OK to consider that user error, and give inconsistent results?
  • in order to handle duplicates, we need to build a map. We then convert the rootStrings to paths, and put them in a ChildNamesMap which then builds its own map. This is useful if two roots have the same leaf name ... but it's pointless if all prototypes come from the same parent location ( in which case they must already have unique names ) ... and that's probably by far the most common case?

Anyway, after contemplating whether there was any better solution that could address more of these issues, I figured I should forget about it for now, and just do the basic fix that makes this case at least work, even if it isn't optimal.

Comment on lines 1296 to 1299
if indices:
points["root"] = IECoreScene.PrimitiveVariable( points["root"].interpolation, roots, indices )
else:
points["root"] = IECoreScene.PrimitiveVariable( points["root"].interpolation, roots )
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? As far as I can tell, it's fine to pass None for the last argument to PrimitiveVariable().

@danieldresser-ie danieldresser-ie force-pushed the instancerUnindexed branch 2 times, most recently from 6aef05e to 6a9547e Compare March 20, 2024 01:16
@danieldresser-ie
Copy link
Contributor Author

Rebased to current 1.3, added a Changelog entry, addressed the comment that didn't get through last year.

If we want something that can be released now, I think sticking with the current fix is probably the best approach. John mentioned via private message that for the currently needed application, we could assume that prototypes are unique when there are no indices, but for the general case, the performance hazard of unnecessarily duplicating prototypes seems worse than the hazard of checking for duplicates only to find that there weren't any. Most of my original concerns were that it feels like there might be an overall better way to refactor some of this, but that's a fairly philosophical concern. If we just want to make sure that as many cases as possible do something reasonable, I think I agree with Daniel from last year that this is a reasonable approach.

@johnhaddon
Copy link
Member

Thanks Daniel!

@johnhaddon johnhaddon merged commit 28976d9 into GafferHQ:1.3_maintenance Mar 20, 2024
4 checks passed
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.

2 participants