Instancer hash bug #6258
Draft
Instancer hash bug #6258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We still catch these invalid prototype paths, but we do it slightly deeper in the hierarchy
This was referenced Feb 12, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've added a test and a fix for the instancer hash bug I noticed.
Seems reasonably straightforward, and moves things a bit in the direction we want to go anyway to support relative prototype paths.
The main loss of performance is that we now compute the engine while hashing childNames for ".../instances/" ... but in order to evaluate that, we must have already evaluated the childNames one level above, which would have evaluated the engine anyway. So this doesn't seem to be a problem in practice. I quickly hacked together a test to show some performance loss, which shows 15%, but this loss is completely negligible in any of the more practical tests.
I've made this a draft PR because I don't think it makes sense to merge this before we figure out how it fits in with all the other Instancer stuff we're looking at now, but in isolation, I think it could be quite a reasonable change.