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 a button to clear curve points in the Path2D editor #81325

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Add a button to clear curve points in the Path2D editor #81325

merged 1 commit into from
Jan 5, 2024

Conversation

AttackButton
Copy link
Contributor

@AttackButton AttackButton commented Sep 5, 2023

  • Create a get_points function in scene\resources\curve.h.
  • Create clear_curve_points and restore_curve_points to handle the undo_redo system in the editor\plugins\path_2d_editor_plugin.h.
  • Set the MODE_EDIT when entering the Path2D node, if a curve has already been created.
  • Set the MODE_CREATE if the curve is valid, but empty.
  • Added a confirmation dialog before clearing the points.
  • Added the possibility to delete points in edit and edit curve modes.

clear_points_test

@AttackButton AttackButton requested a review from a team as a code owner September 5, 2023 04:28
@Chaosus Chaosus added this to the 4.2 milestone Sep 5, 2023
@AttackButton AttackButton marked this pull request as draft September 5, 2023 09:01
@YuriSizov YuriSizov requested a review from a team September 5, 2023 09:55
@MewPurPur
Copy link
Contributor

It's a bit more of a "danger" action and also the only one that does what you need in one click instead of needing to interact with the 2D editor. Maybe it should be in the Options dropdown instead?

@AttackButton
Copy link
Contributor Author

I'm trying to close all the possibilities of undo_redo first (the hard part of the pr) to discuss this. I believe that the button is so different from the others and the user can still use ctrl+z.

@KoBeWi
Copy link
Member

KoBeWi commented Sep 6, 2023

I think moving to to Options menu is better for 2 reasons:

  • it's not something used often
  • other buttons are for interacting with the path, this one isn't (although this applies to close curve too I guess...)
    Making destructive action more difficult to click accidentally is a bonus, but undoredo makes it irrelevant.

@AThousandShips
Copy link
Member

You need to resolve the conflicts

@AttackButton
Copy link
Contributor Author

AttackButton commented Sep 7, 2023

History working and added a confirmation dialog before clearing the points.

clear_points_dialog

@AttackButton AttackButton marked this pull request as ready for review September 7, 2023 06:01
@AttackButton AttackButton marked this pull request as draft September 7, 2023 17:32
@AttackButton

This comment was marked as outdated.

@AttackButton AttackButton marked this pull request as ready for review September 7, 2023 20:09
@AttackButton AttackButton marked this pull request as draft September 7, 2023 20:44
@AttackButton AttackButton marked this pull request as ready for review September 8, 2023 01:23
@AttackButton
Copy link
Contributor Author

Just finished. It's ready for review.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 2, 2023

The implementation looks alright, but not sure about the auto mode change. It might be annoying when editing multiple paths and you want to use another tool.

Also the clear operation doesn't need confirmation, because you can undo it.
Also I still think the popup menu might be better place to put it.

@AttackButton
Copy link
Contributor Author

AttackButton commented Oct 2, 2023

I agree with the auto mode change part. In fact, when editing multiple curves you may want to stick with the same mode. I'm going to remove it at least from the part when you select the Path2D node in the editor.

As a user, I can say that leaving this button exposed in the editor is very important. This may prevent someone from intuitively selecting all the curve points on the screen and pressing delete. Action that would attempt to delete the node from the tree.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 3, 2023

The second part of this comment was not addressed: #81325 (comment)

@AttackButton
Copy link
Contributor Author

AttackButton commented Oct 3, 2023

The second part of this comment was not addressed: #81325 (comment)

As I said before, leaving this button exposed is important because without it the user will not know how to clear the points. The confirmation dialog is necessary as the button will be exposed to users; furthermore, it resembles the node deletion action.

@AttackButton AttackButton reopened this Oct 3, 2023
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks ok.
Feature has no proposal.

@akien-mga akien-mga modified the milestones: 4.2, 4.x Oct 3, 2023
@AttackButton

This comment was marked as off-topic.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems useful to me.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Jan 4, 2024
@akien-mga akien-mga merged commit dc5b57e into godotengine:master Jan 5, 2024
15 of 23 checks passed
@akien-mga
Copy link
Member

Thanks!

@Lu-TheCoder
Copy link

Hmm should this be happening??
I think something is broken...

issue.mp4

Godot 4.3 dev 5
Running on Mac M1

@KoBeWi
Copy link
Member

KoBeWi commented Apr 21, 2024

That's #89781
Will be fixed in dev 6.

@AttackButton AttackButton deleted the path_2d_editor_plugin-clear_points branch July 1, 2024 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants