Reveal selected samples from a selected node in a tandem plot #243
Conversation
|
This is really exciting! Haven't gone through the code yet but when I click on the root node I get the following browser error:
... and, after this, I can't select any nodes (the menus don't even come up; I just get the same error again, for both tips and internal nodes):
Looks like Emperor is breaking somewhere? Not sure. |
There was a problem hiding this comment.
This looks cool. I have a few (mostly minor) suggestions, but once those are addressed (and once the matching PR (#237) is merged in to master and we verify that it fixes the issues with this I brought up earlier), then this should be good to merge.
empress/core.py
Outdated
| @@ -24,6 +24,8 @@ | |||
| TEMPLATES = os.path.join(SUPPORT_FILES, 'templates') | |||
| SELECTION_CALLBACK_PATH = os.path.join(SUPPORT_FILES, 'js', | |||
| 'selection-callback.js') | |||
| TIP_CLICK_CALLBACK_PATH = os.path.join(SUPPORT_FILES, 'js', | |||
| 'tip-click-callback.js') | |||
There was a problem hiding this comment.
This callback is called tip-click-callback, but it looks like this information is also sent over for internal nodes. Should this just be renamed to node-click-callback or something?
There was a problem hiding this comment.
Sorry, it looks like this conversation was marked as resolved but I don't think this change was applied...
|
|
||
| ec.decViews.scatter.setEmissive(0x000000, samples); | ||
| ec.sceneViews[0].needsUpdate = true; | ||
| }); |
There was a problem hiding this comment.
This code looks OK, but would suggest renaming the module to node-click-callback or something (see the comment in core.py).
There was a problem hiding this comment.
(sorry, this was marked as resolved but i don't think has been addressed... if i'm misunderstanding something just let me know)
Co-authored-by: Marcus Fedarko <[email protected]>
|
@fedarko should be ready for another pass, thanks! |
There was a problem hiding this comment.
There are a couple unresolved things that I don't think have been addressed? Once those are handled this should be fine. (I remember this happened a while back, and it turns out that GitHub just broke or something for me and wasn't showing the updated things... but I pulled all of the latest code and I think some things really are missing. But if I'm wrong, just let me know.)
Thanks!
empress/core.py
Outdated
| @@ -24,6 +24,8 @@ | |||
| TEMPLATES = os.path.join(SUPPORT_FILES, 'templates') | |||
| SELECTION_CALLBACK_PATH = os.path.join(SUPPORT_FILES, 'js', | |||
| 'selection-callback.js') | |||
| TIP_CLICK_CALLBACK_PATH = os.path.join(SUPPORT_FILES, 'js', | |||
| 'tip-click-callback.js') | |||
There was a problem hiding this comment.
Sorry, it looks like this conversation was marked as resolved but I don't think this change was applied...
|
|
||
| ec.decViews.scatter.setEmissive(0x000000, samples); | ||
| ec.sceneViews[0].needsUpdate = true; | ||
| }); |
There was a problem hiding this comment.
(sorry, this was marked as resolved but i don't think has been addressed... if i'm misunderstanding something just let me know)
empress/core.py
Outdated
| filter_extra_samples, | ||
| ignore_missing_samples, |
There was a problem hiding this comment.
this order disagrees with the parameter names in validate_and_match. I got around this by switching the param order in tools.match_inputs() as done in this other branch's commit:
Lines 58 to 67 in aa40f2b
Co-authored-by: Marcus Fedarko <[email protected]>
Users can now reveal selected samples by clicking on a node in the tree. Selections are shown when a node is clicked, and hidden when the node's menu is removed from the screen.
This was paired-programmed with @kwcantrell 🥇