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

Clarify port/slot distinction and add method to get slot idx from port idx in GraphNode #40043

Closed

Conversation

mrushyendra
Copy link
Contributor

The GraphEdit class documentation uses the terms port and slot interchangeably to refer only to slots that are enabled,
whereas the GraphNode class documentation uses slot to refer to any slot, regardless of whether it is enabled or not.
This PR clarifies the distinction between ports and slots in the GraphEdit class documentation, and updates the relevant
signal descriptions accordingly.

It also adds a method to convert a port/connection index to its corresponding slot index on the GraphNode.

Closes #37227

@mrushyendra mrushyendra requested a review from a team as a code owner July 2, 2020 06:11
scene/gui/graph_node.cpp Outdated Show resolved Hide resolved
Clarified distinction between ports and slots in GraphEdit class
documentation, and added method to convert a port index to its
corresponding slot index.
@fire
Copy link
Member

fire commented Jul 2, 2020

In Visual Script one of the terms means the entire left and right connectors. The other one is the "sequence" connectors and the function parameter connectors. To get the proper function parameter connectors you need to subtract the sequence connectors.

Hopefully that gives flavour to your documentation.

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 3, 2020
@mrushyendra
Copy link
Contributor Author

@fire Should that information go into the VisualScriptNode/VisualScript pages? Or are you envisioning it more like an additional clarification in the GraphEdit page?

@Chaosus
Copy link
Member

Chaosus commented Sep 29, 2020

I would like to leave a 'port' term everywhere and remove 'slot' entirely (I don't much like this word since only one letter divided it from one vulgar word)

@umarcor
Copy link
Contributor

umarcor commented Sep 29, 2020

I'd like to have port used consistently too. In the context of Hardware Description Languages, such as VHDL, ports are known to have a mode (which might be in, out or inout) and a type. Only ports of the same type can be connected to each other, and ports of mode 'in' or 'out' are unidirectional. I feel that the terminology could be directly applied to GraphNode. The current usage of left and right instead of input and output is only valid as long as the layout is constrained to a left-to-right flow.

@fire
Copy link
Member

fire commented Sep 29, 2020

Can you write a proposal what to rename it to? I don't think removing the term is proper.

I like @umarcor 's proposal, I tried to find how the ISO standard defines graphical node line graphical language, so it matches the terms. However, it was difficult, and I couldn't find it.

@umarcor
Copy link
Contributor

umarcor commented Sep 29, 2020

@fire, note that I am currently trying to prototype an schematic editor for structural HDL designs (see umarcor.github.io/hwstudio and godotengine/godot-proposals#1401). Hence, my proposal is very biased. I'd like to hear other users describe use cases where it might not fit, before making a stronger proposition.

@akien-mga
Copy link
Member

A lot of time has passed, but it would be good to revisit this and try to reach a consensus on the terminology. CC @godotengine/gui-nodes

@clayjohn
Copy link
Member

clayjohn commented Aug 9, 2022

We took a look in the PR review meeting today. This needs a more in-depth review from knowledgeable contributors cc @fire @Chaosus @YuriSizov

@YuriSizov
Copy link
Contributor

To add to that, we should resolve this for 4.0, because it can end up compatibility breaking and that would be very annoying to keep for another major release cycle.

@YuriSizov
Copy link
Contributor

Thanks for your contribution! I went ahead and remade the PR against the current master and update it to better fit Godot's engine design. Your work has been credited as a co-author!

Superseded by #65574.

@YuriSizov YuriSizov closed this Sep 9, 2022
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Sep 10, 2022
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.

Graph node confusing slot indexing
7 participants