[4.x] Sorts AnimationNodeBlendTree node connections for deterministic output

[LoparPanda]

This should resolve #42788.

Is there a way to pull off the sort without the temporary String copies? I wasn't sure how StringName is set up but the == and < operators seem to be just comparing the inner _data pointers, so unless I missed something, the pointers would have to be pre-sorted in memory for that to be used for comparison.

Mirroring this in a 3.x PR as well.

[fire]

Can you read the https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html? In particular the squashed guideline.

I'm interested in reviewing this. @TokageItLab too.

[LoparPanda]

Can you read the https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html? In particular the squashed guideline.

I'm interested in reviewing this. @TokageItLab too.

Done! Should be all squashed now on both PRs.

[AThousandShips]

LGTM, haven't tested but makes sense

[TokageItLab]

This problem is caused by the fact that the HashMap nodes do not have an order.

However, changing it to RBMap nodes did not solve the problem. When I look at the RBMap nodes when adding a node to add_node(), it appears that the insertion into the RBMap is not done uniquely by key. @reduz Do you know anything about this?

So why is this problem not happening in other resources?

For example, VisualShader also uses GraphNode, but VisualShader's connections are stored for each node in a child of the "nodes/" path.

On the other hand, the AnimationBlendTree stored it in an ordered array called "node_connections", even though the nodes are unordered. However, since the nodes are sorted and stored in .tres, this problem can be solved by storeing "node_connections" as children of "nodes" means "nodes/XXX/connections". (Then, conversion code is needed for getter and setter for compatibility)

So, this PR's solution does not seem so smart, it may be applicable to 3.x, but at least, what needs to be sorted is the order of the nodes, not the input_nodes. Also, it should only be needed when recording to .tres so it needs to be in _get() (this fix should not be necessary for BlendTreeEditor, etc.).

[AThousandShips]

So, this PR's solution does not seem so smart

This isn't really a called for phrasing and isn't very constructive

[TokageItLab]

Let me rephrase.

Sorting where it is not needed is not good for performance. Also, this fix does not address the fundamental problem and is just a stopgap, and the implementation is inconsistent when compared to other classes.

[TokageItLab]

I tried to store it in "nodes/XXX/connections". Indeed, it has unique order. However, the result seems to be that it has to be kept separately as "node_connections". This is because there is a restriction that "connections" must be loaded after all other nodes.

Also, when I investigated again why RBMap works well in VisualShader, I found that it uses int id as the key. So, apparently there is a problem with RBMap<StringName, V>, but RBMap<String, V> definitely works well.

I sent #79595 which can supersede this PR.

[TokageItLab]

Superseded by #79595.

[AThousandShips]

Thank you for your work, and hope to see other contributions from you going forward, even if this didn't pan out!