Skip to content
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

add command to group selected nodes #14

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

NathanLovato
Copy link
Contributor

@NathanLovato NathanLovato commented Jun 1, 2023

This PR aims to add group and ungroup commands for selected nodes.

Close #10

@NathanLovato NathanLovato marked this pull request as draft June 1, 2023 19:53
@NathanLovato
Copy link
Contributor Author

I have short timeframes to work on contributions like these (~30mn at a time), so it'll take me some time to complete this. I'm starting by trying to make the command work well destructively before converting to undo and redo

@NathanLovato NathanLovato force-pushed the nathan/group-nodes branch 2 times, most recently from fe2f410 to b614374 Compare June 2, 2023 07:01
@NathanLovato
Copy link
Contributor Author

Grouping is now working. There's an error that triggers for some nodes that I need to look into, probably due to the order of the undo redo methods. It's a minor thing, doesn´t prevent the feature from working.

Next up is ungrouping.

@NathanLovato
Copy link
Contributor Author

Hi, I added ungrouping this morning. There are still some non-blocking undo errors to fix before merging, but could you tell me if the behavior is what you'd expect? I made a short recording with voice to showcase how it works right now:

https://www.dropbox.com/scl/fi/folfj182wbru6xg06wij8/2023-06-03-grouping-and-ungrouping.mp4?dl=0&rlkey=34o7o111yizp5huj9227ik4ew

Please let me know if you'd like anything to work differently as part of this PR.

When I next have a moment to work on this I'll work on fixing the undo errors that appear in the output.

@NathanLovato NathanLovato marked this pull request as ready for review June 3, 2023 07:04
@imjp94
Copy link
Owner

imjp94 commented Jun 3, 2023

Yes, this is working great!
But I just push a commit for:

  • Maintain order of grouped nodes after undo/redo
  • Ensure name of group node always readable
  • Select group node after grouping

Let me know if you are fine with the changes

2023-06-03.23-24-24.mp4

Not sure if this happens to you, but sometime it just doesn't work. It's pretty random, I am not sure what is causing this bug.

2023-06-03.23-13-20.mp4

@NathanLovato
Copy link
Contributor Author

Thanks for adding changes, they're great!

Not sure if this happens to you, but sometime it just doesn't work. It's pretty random, I am not sure what is causing this bug.

I think it's because I put the input code in _forward_gui_input(), so the shortcut only works when the mouse cursor is hovering the viewport. Moving the code to _input() should fix this.

@NathanLovato
Copy link
Contributor Author

Just fixed it!

@imjp94
Copy link
Owner

imjp94 commented Jun 3, 2023

All good!
Should be ready to merge after the errors are resolved

@NathanLovato
Copy link
Contributor Author

I could fix the errors when ungrouping nodes. There's one kind of error I don't know how to fix right now: UndoRedo history mismatch: expected 0, got 2.

I'll be away for ~2 days but I'll look into it once I'm back

@NathanLovato
Copy link
Contributor Author

I'm sorry for the delay, we're swamped with work. It's still on my todo but we have to prioritize our work to ensure we have enough money until the end of the year. So I'm not forgetting this, I'll try to get to it as soon as possible.

@imjp94
Copy link
Owner

imjp94 commented Jul 3, 2023

Don't worry, just work in your timeline =)

@NathanLovato
Copy link
Contributor Author

NathanLovato commented Feb 26, 2024

I hope you'll excuse the very long delay! I've been in a 6+-months crunch, that's still ongoing actually, and I didn't get much time for personal projects (like this contribution).

I just got back to this group/ungroup feature and could fix the "UndoRedo mismatch" error: it was due to the order of undo-redo operations, which matter to the system.

Now, it should be good to go. Let me know if thereś anything more you'd like me to change.

@imjp94
Copy link
Owner

imjp94 commented Feb 28, 2024

Don't worry about that, just take you time =)
These projects are pretty much in "maintenance mode", I am grateful that there are many contributors contributing bugfix and adding new feature for me.

But there are some issues with ungroup node, which I might had overlooked last time:

  • Undo "Ungroup Group Node" doesn't bring back the group node and its children
undo-ungroup-error.mp4
  • Children of "group node" are not inserted to "group node" position after ungrouped. When ungrouping nodes. The "group node" is removed and its children are added to the back of the list. I think it would makes more sense if the children are inserted at "group node" original position.

Btw, I have pushed a commit to remove the "!is_inside_tre()" error when undo "Group Node".

@NathanLovato
Copy link
Contributor Author

Thanks for taking the time to test and review :). Great catches!

I'm keeping this in my open Github notifications. I've got to find time to tackle it. I'll get to it as soon as possible (which, probably, isn't gonna be very soon..!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: "group" and "ungroup" selected objects
2 participants