Hide/show items on double click #18
Conversation
…an item The mechanism is to pass tree object to qt node item to be used on the double click callback. The qt node item also receives the uuid and store it for the callback. The callback will change the item visibility (new variable on visibility.py) and trigger a refresh, redraw. The visibility now uses str() instead of unique_id.fromMsg to be able to compare the id that is stored on the node_item names Also triggered the refresh/redraw for the _update_visibility_level() Known bug: Sometimes there is a segmentation fault after the double click. It can happens when double click is executed. Always happens when fit is enabled and the root is double clicked to hide, then it's double clicked to show again. The segmentation fault happens after the signal redraw slot is executed.
…tion fault Now a signal is passed to the node item with QueuedConnection.
There was a problem hiding this comment.
Nice work - this code isn't the easiest to get around. Your approach is not too dissimilar to what I ended up doing in the JS / ROS2 version in splintered-reality/py_trees_js#50. Two differences that I can see:
- We changed the visual for the parent that was clicked - grokking what was collapsed and what wasn't was a cognitive load. The original PR used a linear gradient, later versions added a symbol to the node to highlight it more clearly.
- We marked children as 'not visible' at click-time instead of recursive redraw-time lookups. Slightly more optimal.
I don't think either of these are showstoppers for you here though. Highlighting the parent can be a follow-up PR if you find it helps you and I don't think your recursive lookups are going to slow it down.
As for branch - here is fine for melodic. devel is currently sync'd to the noetic release. Just make sure to leave us an issue floating around to remind us it can be ported forward to noetic.
Next Steps:
- Clean up the import spaghetti
- Check the timings on the redraw for a largish graph, be nice to confirm it's not slowing it down appreciably
- (myself) I'll test the code when I'm back at my home PC where I have a melodic install (~Jan 15)
| from qt_dotgraph.dot_to_qt import DotToQtGenerator | ||
| from qt_dotgraph.pydotfactory import PydotFactory | ||
| from qt_dotgraph.pygraphvizfactory import PygraphvizFactory | ||
| from .qt_dotgraph.dot_to_qt import DotToQtGenerator |
There was a problem hiding this comment.
Ach, ugly smashing of python import styles here. Not unexpected - this app is still proof-of-concept with rough edges, no linting in CI and the result of a few people dipping into it. Not the end of the world - I might only ask that, regardless of the rest of the app, we at least get it consistent in this module.
- Local relative imports with .'s
- One of
from . import mymoduleorfrom .mymodule import mything
I usually prefer the former since it makes the code more easily traceable internally, but given that it's predominantly the latter, perhaps just drop the from . import visibility above and make sure you use from .visibility import ... (i.e. with dots to signify it's a local import) for any methods/classes used here.
|
I work with a tree that can be very big and is already slow to update, so I'll check the performance with the feature. If it's bad it should be easy to change to "children as 'not visible' at click-time". The reason that I didn't go that approach is because a tree node could have its parent changed, and this change wouldn't be tracked on the rqt_py_trees when a new message arrives. I don't work with something that does that. Do you think that this is relevant? One thing I noticed is that I should reset the The highlight is pretty nice. I could try something similar to what was done on the splintered-reality/py_trees_js#50 or the symbol, but it would be on another PR. I hope I can do the tests/changes this week. |
That'd be great (A/B comparison) - I only have the tutorials on hand to work with (all my current company's work is in py_trees 2.x), so it doesn't use the rqt_py_trees viewer.
Hmm, dynamic insertion and pruning I've always taken care to enable, but I didn't even consider relocation. I can't think of any use case that requires it, nor would I advise it in terms of design. It introduces a great deal of complexity with no benefit that I can determine.
Oh, good catch. I'd just shoot for the reset as I haven't heard of anyone flipping back and forth on topics yet (and you're not breaking the view, just losing a UI convenience).
SGTM
No rush. I might be able to setup an nvidia docker container running melodic to play around with this. |
Hi @stonier
I created a feature as #13 because I needed for my work. I'm using melodic. I'm not sure if this is the best way to implement it but can you check it out?
To implement it, I considered that the safest thing to do would be to record the ids of what is to be hidden because that could be used when a new message is received.
visibility.pyto store the ids that should be hiddenis_visible()to see if one of the parents of a node is hiddenbehavior_tree.pyvisibility.pyI think there isn't much of a point to save the ids of what should be hidden because if the tree restarts, it will generate new unique ids.