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

Wrong number of newly added output pin on Sequence node #131

Closed
F0bRE opened this issue Feb 7, 2023 · 1 comment
Closed

Wrong number of newly added output pin on Sequence node #131

F0bRE opened this issue Feb 7, 2023 · 1 comment
Assignees
Labels
bug fix Something isn't working

Comments

@F0bRE
Copy link

F0bRE commented Feb 7, 2023

Found this bug on the 1.3 version for UE 5.0.

If you add new output pins to the Sequence node, delete one of the pins (except last one) and then try to add new pin, the newly added pin will have wrong number. It will duplicate the number of the last pin.

For example, we add pins 2 and 3 to the Sequence node:
image

Remove pin 2:
image

Add new pin:
image

When this Sequence will be executed, only one of identically named output pins will be triggered:
image

As far as I understood, problem is in function void UFlowGraphNode::AddInstancePin, in the row const FFlowPin PinName = FFlowPin(FString::FromInt(NumberedPinsAmount));.
It names new pin according to the number of output pins, with disregard of last pin's number.

So in my example, we have output pins 0, 1 and 3. There are three output pins, so the newly added will have PinName = 3, and we will have output pins 0, 1, 3 and 3.

@MothDoctor MothDoctor self-assigned this Feb 18, 2023
@MothDoctor MothDoctor added the bug fix Something isn't working label Feb 20, 2023
@MothDoctor
Copy link
Contributor

Fixed. Now pin names below removed pin should be correctly updated :)

MothDoctor added a commit that referenced this issue May 26, 2023
* formatting fix

* Fixed rare issue with loading saves - prevent triggering input on not-yet-loaded node

* wording corrected

* extracted graph-related code from the FFlowAssetEditor to new SFlowGraphEditor class

added FFlowAssetEditor::IsTabFocused() to prevent delete/paste/copy nodes if graph tab isn't focused

* fixed graph commands

* #131 fixed updating pin names below removed pin

* quick CIS compilation fix

* another CIS fix attempt

* redundant include removed

* fixed showing static descriptions in PIE/SIE

renamed flag to better reflect its role

* add null flow asset check when calling StartRootFlow (#132)

Co-authored-by: Krzysiek Justyński <krzysiek.justynski@gmail.com>

* Add custom color support to nodes (#135)

* readability tweak

* non-editor compilation fix

* Added includes to fix compiler errors

When compiling for UE 5.1 (Win64) we ran into some compile errors due to missing header includes.

* Added includes to fix compiler errors (#152)

When compiling for UE 5.1 (Win64) we ran into some compile errors due to missing header includes.

* Fixed bug where Owner parameter wasn't being used

Owner was passed into this function, presumably to be used instead of 'this', but it was just using 'this'.

Changed the code to use Owner if specified, and default to 'this' otherwise.

* Fixed bug where Owner parameter wasn't being used  (#153)

* Added includes to fix compiler errors

When compiling for UE 5.1 (Win64) we ran into some compile errors due to missing header includes.

* Fixed bug where Owner parameter wasn't being used

Owner was passed into this function, presumably to be used instead of 'this', but it was just using 'this'.

Changed the code to use Owner if specified, and default to 'this' otherwise.

* comforting convention of separating engine headers from plugin headers

* Extending the "GetAssignedGraphNodeClass()" function to support to return the correc "EdGraphNode" for grand child classes of "UFlowNode" (#151)

Previously it would return the first found "IsChildOf" and return the EdGraphNode of the foundclass.

But lets say we got this situation just for example:
UFlowNode->UMyBaseNode
UFlowNode->UMyBaseNode->UMyQuestNode

Both "UMyBaseNode" and "UMyQuestNode" could be in the list if we want different EdGraphNodes for both of them. If the "UMyQuestNode" goes into the itteration and it would find "UMyBaseNode" first it will be returned without checking the rest, because "UMyQuestNode" is dirived from "UMyBaseNode" (IsChildOf == true) and thus it returns the EdGraphNode of the base node directly. So that would mean I got the wrong EdGraphNode for my special quest node.

This modification continues to try and find parent classes and returns the closest parent version.
So lets say we have this situation:

UFlowNode->UMyBaseNode
UFlowNode->UMyBaseNode->UMyQuestNode->MyOtherQuestNode(BP class)
Both “UMyBaseNode” and “UMyQuestNode” would be found, but the closest parent to “MyOtherQuestNode” would be returned, in this case the EdGraphNode of “UMyQuestNode”

It only does this if we have found 2 or more parents though.
If only 1 is found we would return that one, and if the exact class match is found lets say: "UMyQuestNode" finds "UMyQuestNode" it will return the EdGraphNode right away aswell.

Hopefully this makes sence :P Otherwise feel free to ask.

* Show pretty readable pin name even if friendly name is not provided (#140)

* Show pretty readable pin name even if friendly name is not provided

* Add missing else. Silly mistake 😋

* cosmetic tweaks of last PR

* rework of PR #140

- added bEnforceFriendlyPinNames as separate flag from the Unreal editor config
- moved creating a display name to `UFlowGraphSchema::GetPinDisplayName` without modifying graph node's instance

* CustomInput & Output Details Customization and other cleanup

* Added forward declarations & includes that were needed to compile
* Changed some functions to return const &'s rather than doing a full deep copy of a member container
* Explicitly calling MoveTemp() to use move semantics (supported by UE container classes (eg TArray)) to move the array without an extra copy.  Some compilers might be able to do this optimization automatically, but being explicit here.
* Introduced a superclass to UFlowNode_CustomInput and Output.  The diff is easier if you compare them to the new superclass, since most of the code moved into it.
* The "details" superclass for CustomInput/Output fixes a discovered bug where the list of EventNames was not recaching correctly.
* Moved a bit of code from PlayLevelSequence to FlowNode.h (TryGetRootFlowActorOwner) so that it can be used in other contexts.  Also provided a component version of this same code.
* The FlowGraphSchema will now create default nodes for any CustomInputs that exist when the asset is first created.
* Added a UseAdaptiveNodeTitles option to optionally make CustomInputs integrate their EventName into the title for the node.  This defaults to false (to preserve previous behavior by default).
* Exposed CustomInput Add/Remove functions on UFlowNode to allow subclasses to modify the CustomInputs array.

* Additional includes for compiler errors

* Update FlowAsset.cpp

non-editor configurations compile fix for previous changelist.

* Removing MoveTemp for local variables

According to this guidance, it's no longer a performance improvement.
 https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f48-dont-return-stdmovelocal

* Extended CustomOutputs and other quality of life improvements

- Custom Outputs now can be listened to by the owning component

- Added a DeinitializeInstance() call to FlowNode (called when its FlowAsset instance owner is deinitialized)

- Added Edit Asset Defaults button to Flow Editor

* Code fixes after merge with latest

* Removed these replaced files

* Moves some stuff around

* tweaks

* revert this

---------

Co-authored-by: Krzysiek Justyński <krzysiek.justynski@gmail.com>
Co-authored-by: Bohdon Sayre <bohdon@gmail.com>
Co-authored-by: Satheesh (ryanjon2040) <ryanjon2040@users.noreply.github.com>
Co-authored-by: Alex van Mansom <45290811+Vi-So@users.noreply.github.com>
MothDoctor added a commit that referenced this issue Dec 20, 2023
* formatting fix

* Fixed rare issue with loading saves - prevent triggering input on not-yet-loaded node

* wording corrected

* extracted graph-related code from the FFlowAssetEditor to new SFlowGraphEditor class

added FFlowAssetEditor::IsTabFocused() to prevent delete/paste/copy nodes if graph tab isn't focused

* fixed graph commands

* #131 fixed updating pin names below removed pin

* quick CIS compilation fix

* another CIS fix attempt

* redundant include removed

* fixed showing static descriptions in PIE/SIE

renamed flag to better reflect its role

* add null flow asset check when calling StartRootFlow (#132)

Co-authored-by: Krzysiek Justyński <krzysiek.justynski@gmail.com>

* Add custom color support to nodes (#135)

* readability tweak

* non-editor compilation fix

* Added includes to fix compiler errors

When compiling for UE 5.1 (Win64) we ran into some compile errors due to missing header includes.

* Added includes to fix compiler errors (#152)

When compiling for UE 5.1 (Win64) we ran into some compile errors due to missing header includes.

* Fixed bug where Owner parameter wasn't being used

Owner was passed into this function, presumably to be used instead of 'this', but it was just using 'this'.

Changed the code to use Owner if specified, and default to 'this' otherwise.

* Fixed bug where Owner parameter wasn't being used  (#153)

* Added includes to fix compiler errors

When compiling for UE 5.1 (Win64) we ran into some compile errors due to missing header includes.

* Fixed bug where Owner parameter wasn't being used

Owner was passed into this function, presumably to be used instead of 'this', but it was just using 'this'.

Changed the code to use Owner if specified, and default to 'this' otherwise.

* comforting convention of separating engine headers from plugin headers

* Extending the "GetAssignedGraphNodeClass()" function to support to return the correc "EdGraphNode" for grand child classes of "UFlowNode" (#151)

Previously it would return the first found "IsChildOf" and return the EdGraphNode of the foundclass.

But lets say we got this situation just for example:
UFlowNode->UMyBaseNode
UFlowNode->UMyBaseNode->UMyQuestNode

Both "UMyBaseNode" and "UMyQuestNode" could be in the list if we want different EdGraphNodes for both of them. If the "UMyQuestNode" goes into the itteration and it would find "UMyBaseNode" first it will be returned without checking the rest, because "UMyQuestNode" is dirived from "UMyBaseNode" (IsChildOf == true) and thus it returns the EdGraphNode of the base node directly. So that would mean I got the wrong EdGraphNode for my special quest node.

This modification continues to try and find parent classes and returns the closest parent version.
So lets say we have this situation:

UFlowNode->UMyBaseNode
UFlowNode->UMyBaseNode->UMyQuestNode->MyOtherQuestNode(BP class)
Both “UMyBaseNode” and “UMyQuestNode” would be found, but the closest parent to “MyOtherQuestNode” would be returned, in this case the EdGraphNode of “UMyQuestNode”

It only does this if we have found 2 or more parents though.
If only 1 is found we would return that one, and if the exact class match is found lets say: "UMyQuestNode" finds "UMyQuestNode" it will return the EdGraphNode right away aswell.

Hopefully this makes sence :P Otherwise feel free to ask.

* Show pretty readable pin name even if friendly name is not provided (#140)

* Show pretty readable pin name even if friendly name is not provided

* Add missing else. Silly mistake 😋

* cosmetic tweaks of last PR

* rework of PR #140

- added bEnforceFriendlyPinNames as separate flag from the Unreal editor config
- moved creating a display name to `UFlowGraphSchema::GetPinDisplayName` without modifying graph node's instance

* CustomInput & Output Details Customization and other cleanup

* Added forward declarations & includes that were needed to compile
* Changed some functions to return const &'s rather than doing a full deep copy of a member container
* Explicitly calling MoveTemp() to use move semantics (supported by UE container classes (eg TArray)) to move the array without an extra copy.  Some compilers might be able to do this optimization automatically, but being explicit here.
* Introduced a superclass to UFlowNode_CustomInput and Output.  The diff is easier if you compare them to the new superclass, since most of the code moved into it.
* The "details" superclass for CustomInput/Output fixes a discovered bug where the list of EventNames was not recaching correctly.
* Moved a bit of code from PlayLevelSequence to FlowNode.h (TryGetRootFlowActorOwner) so that it can be used in other contexts.  Also provided a component version of this same code.
* The FlowGraphSchema will now create default nodes for any CustomInputs that exist when the asset is first created.
* Added a UseAdaptiveNodeTitles option to optionally make CustomInputs integrate their EventName into the title for the node.  This defaults to false (to preserve previous behavior by default).
* Exposed CustomInput Add/Remove functions on UFlowNode to allow subclasses to modify the CustomInputs array.

* Additional includes for compiler errors

* Update FlowAsset.cpp

non-editor configurations compile fix for previous changelist.

* Removing MoveTemp for local variables

According to this guidance, it's no longer a performance improvement.
 https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f48-dont-return-stdmovelocal

* Extended CustomOutputs and other quality of life improvements

- Custom Outputs now can be listened to by the owning component

- Added a DeinitializeInstance() call to FlowNode (called when its FlowAsset instance owner is deinitialized)

- Added Edit Asset Defaults button to Flow Editor

* Code fixes after merge with latest

* Removed these replaced files

* Moves some stuff around

* tweaks

* revert this

* 5.1 backward-compatible markup

Added some markup to support compiling in UE 5.1.

(the flow.uplugin version will need to be locally adjusted to 5.1)

* need the includes too

* Updates from our project

Added accessors, some refactoring of interfaces and a bug fix in CallOwnerFunction

* FlowNodes respect module accessibility boundaries

* Removed redundant includes

* Removed unnecessary deltas

* Reintroduced blank line

* Removed deltas

* Removed extra deltas

* Removed more deltas

---------

Co-authored-by: Krzysiek Justyński <krzysiek.justynski@gmail.com>
Co-authored-by: Bohdon Sayre <bohdon@gmail.com>
Co-authored-by: Satheesh (ryanjon2040) <ryanjon2040@users.noreply.github.com>
Co-authored-by: Alex van Mansom <45290811+Vi-So@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants