Instancer : Support relative prototype paths #6248
Conversation
danieldresser-ie
force-pushed
the
instRelRoots
branch
2 times, most recently
from
8f5a6a9 to
18b28a8
Compare
danieldresser-ie
force-pushed
the
instRelRoots
branch
from
18b28a8 to
5decadc
Compare
There was a problem hiding this comment.
Sorry Daniel, domestic duties beckon and I haven't quite finished the review on this - need to give a bit more attention to the relative paths commit in particular. Figured it was worth posting what I had in the meantime though.
I pushed a couple of commits on the end to update Cortex and update the env-var name to match, but now #6287 is merged you should be able to rebase and drop the Cortex commit.
Not sure why testRelativePrototypePaths is failing on Linux but not Windows. It is passing for me locally on Linux. Maybe it's obvious to you why?
Cheers...
John
Changes.md
Outdated
| @@ -5,6 +5,11 @@ Features | |||
| -------- | |||
|
|
|||
| - AttributeEditor, LightEditor, RenderPassEditor : Added drag and drop editing. Edits can be created or updated by dropping a value into a cell. Cells representing a set expression or string array can be modified by holding <kbd>Shift</kbd> to append to an existing edit, or <kbd>Control</kbd> may be held to remove from an existing edit. | |||
| - USD : Added automatic expansion of USD PointInstancers at render time. | |||
There was a problem hiding this comment.
Just noting that this will need moving - we released 1.5.6.0 already.
danieldresser-ie
force-pushed
the
instRelRoots
branch
5 times, most recently
from
684f414 to
fb514c5
Compare
|
The switch to using node ids instead of addresses appears to have addressed the test failures. Once you've looked through my replies to comments, we just need to figure out what to do with prototypeRootStorage, and then I'll need to squash it, and sort out Changes entries. |
danieldresser-ie
force-pushed
the
instRelRoots
branch
from
fb514c5 to
2c15c1b
Compare
|
I think all feedback is now addressed, and I've added some changelog entries - in theory, all that's left is squashing. |
Thanks! I pushed a couple of slight tweaks for your consideration, but otherwise all looks ready to squash and merge. We also need to fix the broken layout for the time offset in the Context Variation tab - could you either do that in this PR or open a separate PR for it please? |
This is a more reliable way to uniquely identify nodes. The previous approach led to the risk of inconsistent results if a new node got the same address as a recently freed node ( this was probably quite unlikely in practical use, but appears to have become an issue in some tests ).
…NCER_EXPLICIT_ABSOLUTE_PATHS
I now specify explicit layout indices ( relative to 100 ), for everything on this UI tab. This means that this tricky layout is now completely independent of the physical indices of the plugs, so it is less likely to break again in the future. I've also inserted an extra spacer in order to actually get left alignment behaviour ( I'm not sure why the alignment of the "Quantize" label was previously working without requiring this ).
danieldresser-ie
force-pushed
the
instRelRoots
branch
from
f5121f9 to
2df9813
Compare
|
Squashed in a version of your tweak ( see comment above ), and fixed the broken layout. I understand most of why the layout broke: it was mixing custom widgets with explicit layout indices with plugs with default layout indices, so changing the indices of the plugs caused everything to get offset ( I've addressed this by assigning explicit layout indices to everything on that tab, which should prevent the problem in the future ). But the part I don't get is why the alignment of the column labels broke ... or more specifically I don't get how it worked previously. We were specifying |
I haven't verified, but I assume this will have been down to bb1f432. All good I think. |
I'm deleting the previous discussion here, and starting over, because this is a completely different PR to the first time around.
It is now built on top of #6258, #6272, and #6271, ( in addition to requiring the cortex change ImageEngine/cortex#1451 ). Assuming you want to review this in one go, those other PRs can be closed.
I'm feeling fairly good about this - seems to do what we want, with fairly decent test coverage. This includes performance tests - the one noticeable regression is testPrototypeHashPerf, which went from 0.02s to 0.13s. This test was written specifically to demonstrate worst case behaviour - in practice, we usually need to actually evaluate capsules, not just hash them, and the gain in consistency and performance from reusing capsules when possible should outweigh the cost of the much more accurate hash.
While I'm feeling fairly good, this is a significant change to some code that interacts in fairly complicated ways, so it does call for careful review. If you have access to production data from the users requesting the feature, it would be great to confirm that it works as desired in practice.
The only thing I know still needs doing is writing changelog entries - since this isn't getting merged this week, I'm expecting to need to do some rebasing, so I'll write changelog entries afterwards.