Skip to content

Conversation

@thechayed
Copy link

Likely due to recent updates to Unity, graphs would break when switching to Play Mode. To fix this issue, I added a Port Connection Cache and unique ID for Node Ports, allowing them to be reconnected when needed. This can be done externally, but is also done iteratively in the GraphEditorWindow while it is open.

Some QOL improvements were added to the Graph as well that makes it easier to extend for custom tooling.

@AncientPixel
Copy link

BlueGraph is a excellent package I've integrated with my project but I don't think it's actively being maintained at the moment, might have to fork it. But thanks for your efforts, I hope someone approves your PR. :)

@thechayed
Copy link
Author

BlueGraph is a excellent package I've integrated with my project but I don't think it's actively being maintained at the moment, might have to fork it. But thanks for your efforts, I hope someone approves your PR. :)

It really is~! I personally like it more than Unity’s GTK. it’s way easier to extend, even if it’s not the direction Unity’s going.
I’m super happy I could help a little with the changes I made, even if the PR doesn’t get approved ♥
If you want, I’ll keep tinkering with my fork and add anything fun or useful I come across~

@McManning
Copy link
Owner

BlueGraph is a excellent package I've integrated with my project but I don't think it's actively being maintained at the moment, might have to fork it. But thanks for your efforts, I hope someone approves your PR. :)

Sorry about that, I don't have the capacity at the moment to actively maintain this project outside of PR review.

I really do appreciate this fix, I'll get some time set aside to review this today or tomorrow.

Copy link
Owner

Choose a reason for hiding this comment

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

Might want to drop these empty NodeView Hovered.uss/meta files from your PR

Copy link
Author

Choose a reason for hiding this comment

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

Will do!

Runtime/Node.cs Outdated
OnValidateEvent?.Invoke();

if (GraphID == "!")
return;
Copy link
Owner

Choose a reason for hiding this comment

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

These couple lines with the magic "!" ID don't seem necessary here.

Copy link
Author

Choose a reason for hiding this comment

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

🙈 thanks for catching it! I’ll fix it up.

@@ -0,0 +1,287 @@
using JetBrains.Annotations;
Copy link
Owner

Choose a reason for hiding this comment

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

Will this JetBrains import lead to issues for non-JetBrains users?

Copy link
Author

Choose a reason for hiding this comment

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

No, I don't actually use Jetbrains myself, but Visual Studio keeps automatically adding it as a line in my code when I copy/paste things, which I did with this SerializedDictionary which is what I've used for my other project. I should look into how to disable that functionality.

namespace Remedy.Framework
{
[Serializable]
public class SerializableDictionary<TKey, TValue> : IDictionary<TKey, TValue>, ISerializationCallbackReceiver
Copy link
Owner

Choose a reason for hiding this comment

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

I'd say we want to drop the nested Remedy.Framework namespace to clean this up a bit. If this is from their code and we want to still maintain credit, I'd probably just have some authorship info in the SerializableDictionary's docblock.

Small nit as well but I'd rather be consistent with class name and .cs filename. Both as SerializableDictionary would be good.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry about that, I forgot to change the namespace when migrating the code over to BlueGraph, fixing it now..

/// Tells the Node Caching system to cache the Node to a specific type.
/// </summary>
[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = false)]
public class CacheToAttribute : Attribute
Copy link
Owner

Choose a reason for hiding this comment

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

Do you have an example of how would you use this? So we can add something to the Wiki docs.

Copy link
Author

@thechayed thechayed Aug 18, 2025

Choose a reason for hiding this comment

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

Okay, sorry for all the edits! I realized my original response was probably too complex.

I added a Node Cache that caches nodes by their type, or by a desired base type/interface using CacheToAttribute. Lookups using the cache are O(1) with no extra overhead compared to the previous methods, and it returns an array for traversal, which is more efficient at runtime.

CacheTo lets you group nodes together by a base type or interface, so you can declare functionality based on a shared type rather than the concrete node type.

- Remove unnecessary magic '!' check in Node.Validate
- Remove JetBrains namespace import and fix namespace in SerializableDictionary
- Ensure class name matches filename for SerializableDictionary
- Clarify usage of CacheToAttribute and node caching logic
@McManning
Copy link
Owner

Started running this in my samples project (https://github.com/McManning/BlueGraph-Samples). Got it to work, sans a few minor adjustments I had to make:

  1. The shorthand syntax = new(); and ??= new(); aren't supported by 2019.3 since it ran an older version C#. I'm not against bumping minimum supported version up - given that was more than 5 years ago at this point - but since there's not a lot that has changed in this PR that demands newer Unity support we could just do something like the following:
        [SerializeField]
        private SerializableDictionary<SerializableType, Node[]> nodeByTypeCache;
        /// <summary>
        /// The nodes Cached in the Graph, by their type so they can be easily queried.
        /// </summary>
        protected SerializableDictionary<SerializableType, Node[]> NodesByTypeCache
        {
            get
            {
                if (nodeByTypeCache == null)
                {
                    nodeByTypeCache = new SerializableDictionary<SerializableType, Node[]>();
                }

                return nodeByTypeCache;
            }
        }

        /// <summary>
        /// The cached connections between nodes from the Schematics Editor in a Dictionary that allows them to be reconstructed if they're lost.
        /// </summary>
        [SerializeField]
        [HideInInspector]
        internal SerializableDictionary<string, string[]> portConnectionCache = new SerializableDictionary<string, string[]>();

        /// <summary>
        /// A Cache pairing Port IDs to their Ports that is concstructed at Runtime to improve Port Lookup time
        /// </summary>
        [SerializeField]
        [HideInInspector]
        private SerializableDictionary<string, Port> runtimePortCache = new SerializableDictionary<string, Port>();

  1. There was a meta file I needed to delete from the branch, otherwise Unity would complain in a loop
image
  1. Couple minor USS bugs with node headers losing color and the inline editors missing a background:
image

Port colors also seem to be broken versus what it should be (https://github.com/McManning/BlueGraph-Samples?tab=readme-ov-file#execution-flow):

image

Still digging into what's causing this one. May not have necessarily been this PR. The USS issues aren't a blocker here for me though, I can push a patch to fix USS easily enough after merging this.

I think just the extra .meta file causing warnings and whether or not we want to patch the syntax a bit to maintain older Unity support that would be left to do here. Looks good otherwise, and super appreciated!

@McManning McManning self-assigned this Aug 20, 2025
@McManning
Copy link
Owner

Still digging into what's causing this one. May not have necessarily been this PR. The USS issues aren't a blocker here for me though, I can push a patch to fix USS easily enough after merging this.

This looks like it's probably Unity compatibility issues as well while I test under 2019:

StyleSheet import: type=Validation, code=InvalidProperty file=Packages/com.github.mcmanning.bluegraph/Editor/Resources/BlueGraphEditor/NodeView.uss:148
    Expected (flex-start | flex-end | center | stretch | auto) but found 'left'
    align-self: left
    
StyleSheet import: type=Validation, code=InvalidProperty file=Packages/com.github.mcmanning.bluegraph/Editor/Resources/BlueGraphEditor/NodeView.uss:156
    Unknown property 'transition' (did you mean 'position'?)
    transition: height 0.5s

If these USS properties were introduced soonish after 2019, I'd probably be fine bumping the minimum supported version of this addon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants