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] Make connections a property #97449

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

Geometror
Copy link
Member

Fixes #97015
After investigating the issue above, I found no obvious reasons why connections shouldn't be a property.

Main changes:

  • The connections array is now a property and can be modified via the inspector (although that is a bit finicky due to the fact that dead/invalid connections are removed after each update, so can't add connections right now through the inspector). This means connections are now properly serialized/deserialized.

Additional refactoring/optimization:

  • Use Vector instead of List internally for connections (this also removes unreadable list element access, e.g. E->get()->...)
  • Make names of local variables of type Ref<Connection> consistent (c -> conn)
  • Method declaration order

Note: In order to avoid breaking compatibility, I kept the name get_connection_list (just for scripting), but since this name is kind of weird and GraphEdit is still experimental we could change it for consistency.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 17, 2024

Use Vector instead of List internally for connections (this also removes unreadable list element access, e.g. E->get()->...)

You can get rid of E->get() in list too, List supports the same iteration as Vector.
If the connections are local GraphEdit (i.e. not passed anywhere), you can use LocalVector.
EDIT:
Nevermind, there is even a getter. So a Vector is fine.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 17, 2024

Editing connections in the inspector is quite buggy. I think the property should either be hidden or readonly (with a note in the docs).

scene/gui/graph_edit.cpp Outdated Show resolved Hide resolved
scene/gui/graph_edit.cpp Outdated Show resolved Hide resolved
@Geometror Geometror force-pushed the ge-connections-prop branch 3 times, most recently from fe9aeab to b4ea09f Compare October 24, 2024 10:01
@Geometror Geometror requested review from a team as code owners October 24, 2024 10:01
@Geometror Geometror force-pushed the ge-connections-prop branch 3 times, most recently from 47b3c06 to 92ce651 Compare October 24, 2024 10:24
@Geometror
Copy link
Member Author

Fixed the editing in the inspector via implementing a new connection "property": keep_alive.
Connections which have keep_alive set to true won't get automatically deleted during redrawing even if they are invalid. This should also be useful in other cases.
Ideally, I would like to wait with this PR until we have structs as part of the API, but if that happens, we'd need to break compat all over the place (or have another solution at hand) so I guess it doesn't matter. Even with a array of dicts, the editing of connections feels quite nice.

doc/classes/GraphEdit.xml Outdated Show resolved Hide resolved
doc/classes/GraphEdit.xml Outdated Show resolved Hide resolved
@Geometror Geometror force-pushed the ge-connections-prop branch from 92ce651 to 46f3b38 Compare October 28, 2024 16:23
@akien-mga
Copy link
Member

Needs rebase.

doc/classes/GraphEdit.xml Outdated Show resolved Hide resolved
doc/classes/GraphEdit.xml Outdated Show resolved Hide resolved
@Geometror
Copy link
Member Author

Rebased.

@Repiteo Repiteo merged commit 017f0eb into godotengine:master Dec 16, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 16, 2024

Thanks!

@Zylann
Copy link
Contributor

Zylann commented Dec 20, 2024

-const List<Ref<GraphEdit::Connection>> &GraphEdit::get_connection_list() const {
+const Vector<Ref<GraphEdit::Connection>> &GraphEdit::get_connections() const {

This is not renamed in the scripting API, but my module uses it, so it kind of breaks compatibility, in addition to making names different between extensions and core (which we try to keep matching with PRs such as #98132).

@Geometror
Copy link
Member Author

@Zylann I guess we can rename it in the scripting API as GraphEdit is still experimental. What do you think?

@Zylann
Copy link
Contributor

Zylann commented Dec 28, 2024

If it's ok to break compat, then that would make sense.

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.

GraphEdit packedScene serialization issue on MenuHbox and connection_lines
7 participants