-
Notifications
You must be signed in to change notification settings - Fork 12
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
Investigate a sync way to access if a Node is connected to a Display #1620
Comments
Updated DisplayedProperty, added DisplayedTrailsProperty that doesn't rely on instances. @zepumph does this resolve the issue? |
Signed-off-by: Michael Kauzmann <[email protected]>
Signed-off-by: Michael Kauzmann <[email protected]>
New changes and file look good. Review comments are done and committed. It does seem quite heavy weight, but it probably isn't too bad in the vast majority of cases (that only have one or two trails anyways).
scenery/js/util/DisplayedTrailsProperty.ts Lines 149 to 156 in c68ad0f
I'm having trouble thinking that this covers all cases. Don't you just want a mode where you are able to "know all trails" that cause this Node to be rendered? Using logical My biggest concern, along the same lines as the above note, is this block here: scenery/js/util/DisplayedTrailsProperty.ts Lines 177 to 181 in c68ad0f
I do not think that we should be exchanging the PDOM parent for the visual one. I guess it depends on what the use case is, but I feel like, for the Property in general, it makes a lot of sense to be able to know all the trails, this means recursing though the PDOMParent AND the visual one. Perhaps a sync conversation would be best. Up to you and let me know! |
From #1615, it can be quite dangerous to listen to the
Node.changedInstanceEmitter
from scene graph code, since Instances are updated inupdateDisplay
, and that function is very dependent on scene graph state not changing as a side effect to that function.@jonathanolson felt like our current usages of
Node.changedInstanceEmitter
could be instead swapped out with logic that knows if a Node is connected. Some of these usages depend on knowing the exact Display instance, but some don't. The investigation will see if there is a performance-friendly way to access this info synchronously (without needing to wait for updateDisplay() to call).Not exactly sure about the priority, but right now all of the usages we have are not buggy given the current cases, but it would be easy for a buggy case to sneak in without us realizing it in the future.
The text was updated successfully, but these errors were encountered: