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

[GraphEdit] Convert to minimap line after getting connection line. #92463

Conversation

Daylily-Zeleen
Copy link
Contributor

Users can change the start and end position of connection line by overriding _get_connection_line().
We should pass the positions in graph to _get_connection_line() consistently, then conver the line for minimap.

Before:

before

After:

after

@Daylily-Zeleen Daylily-Zeleen requested a review from a team as a code owner May 28, 2024 08:52
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_graph_edit_minimap_connection_line branch 2 times, most recently from 0b7acce to 6324af0 Compare May 28, 2024 09:04
@AThousandShips AThousandShips added this to the 4.3 milestone May 28, 2024
@akien-mga akien-mga requested a review from Geometror May 28, 2024 14:12
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

The one problem I have with this solution is that we now need to convert every line point to graph space, not just the start and end point. The minimap is already quite costly.
I'm also unable to reproduce your bug. Could you provide a MRP?

scene/gui/graph_edit.cpp Outdated Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_graph_edit_minimap_connection_line branch from 6324af0 to a70442f Compare May 28, 2024 17:49
@Daylily-Zeleen
Copy link
Contributor Author

@Geometror ,here is MRP:
FixGrapEdit.zip


Without this pr:

before

With this pr:

after

@Geometror
Copy link
Member

Well, just to be clear, this specifically is not the intended usage.

Users can change the start and end position of connection line by overriding _get_connection_line()

The system was designed so that you can provide a custom line "shape", not offsetting the start and end positions of connections.

That said, you're right, there are a few valid cases that come to my mind where it currently breaks. E.g.:
image
(Unity Shadergraph) Where you would want a straight line segment before/after each port with an absolute length.
To avoid recalculating this transform, we could add another method to the API (something along _get_minimap_connection_line), but I'm not really a fan of that. So this solution seems fine to me :)

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Nice work :)

scene/gui/graph_edit.cpp Outdated Show resolved Hide resolved
scene/gui/graph_edit.cpp Outdated Show resolved Hide resolved
@Daylily-Zeleen Daylily-Zeleen force-pushed the daylily-zeleen/fix_graph_edit_minimap_connection_line branch from a70442f to 28a2fa4 Compare May 29, 2024 04:59
@akien-mga akien-mga merged commit b60471f into godotengine:master May 29, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Daylily-Zeleen Daylily-Zeleen deleted the daylily-zeleen/fix_graph_edit_minimap_connection_line branch June 4, 2024 06:38
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.

4 participants