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

[FEATURE] Get caller instance object of event Raisers #376

Open
TheXRMonk opened this issue Nov 28, 2022 · 15 comments
Open

[FEATURE] Get caller instance object of event Raisers #376

TheXRMonk opened this issue Nov 28, 2022 · 15 comments
Labels
enhancement New feature or request

Comments

@TheXRMonk
Copy link

Is your feature request related to a problem? Please describe.
When debugging only the stacktrace is available - from which you can get no info on which instance of the type raised an event.

Describe the solution you'd like
I assume (from limited research) it's impossible to get the instance of the caller, but I would really like to be able to click a button in the event stacktrace debugger to highlight the instance object that made the raise. Like when you click the log message in the console to get pointed to the context.

Describe alternatives you've considered
Making my own AtomAction that I forcefully put on every gameobject that Raises any event, just to log a console message with the context to the GameObject.

@TheXRMonk TheXRMonk added the enhancement New feature or request label Nov 28, 2022
@soraphis
Copy link
Collaborator

soraphis commented Nov 28, 2022

We can't do it via reflection (there is no way to find the calling object).

So only way would be to change the API for the AtomEvent.cs Raise method to accept a sender as an argument.

this needs to be passed through to the StackTraceEntry into a new member field "_sender".

In the StackTraceEditor as additional callback action the object can be pinged.


Since it most likely will be a breaking change, this would need to wait for the next major release though.
Alternatively we could overload the Raise method.

@TheXRMonk
Copy link
Author

TheXRMonk commented Nov 29, 2022

only way would be to change the API for the AtomEvent.cs Raise method to accept a sender as an argument.

Wouldn't this break the ability to raise AtomEvents (with parameters) from unity events in the inspector? (Can't render more than one method parameter)? E.g. you have to call Raise<T>(Component _sender, T value) which can't be rendered in the inspector?

Unless we get MultipleParameterEventsInspectors (please unity..)

@TheXRMonk
Copy link
Author

An alternative would be to use the new Dependency viewer to find all used references of an Atom as a starting point for debugging issues;
image
However, the package is still experimental and just renaming an Atom breaks the dependency tree, forcing a rebuild, which then doesn't always captures all references within scenes. And even if this worked, I still wouldn't know on a per call basis which object instance Raised the event.

@TheXRMonk
Copy link
Author

TheXRMonk commented Nov 29, 2022

Alrighty, this seems to be the solution I'm going for for now;

public UnityEvent RaiseAtomEvent;

public void RaiseAtom()
{
    if (RaiseAtomEvent.GetPersistentEventCount() == 0)
    {
        Debug.LogError("No Atom assigned to be raised", this);
        return;
    }

    var targetObject = RaiseAtomEvent.GetPersistentTarget(0);

    if (targetObject is not AtomEventBase atomObject)
    {
        Debug.LogError("Only AtomEvents are allowed to be raised by RaiseAtomEvent!", this);
        return;
    }

    string atomType = atomObject.GetType().ToString().Split(".").Last();
    
    Debug.Log("[ATOM] Raising " + atomType + " : " + atomObject.name + " on " + gameObject, gameObject);
    
    if (Application.isPlaying)
        RaiseAtomEvent.Invoke();
}

Along with;

public static void RaiseWithDebug<T>(this AtomEvent<T> atomObject, T value, Object context)
{
    string atomType = atomObject.GetType().ToString().Split(".").Last();
    
    Debug.Log("[ATOM] Raising " + atomType + " : " + atomObject.name + " on " + context.name, context);
    
    if (Application.isPlaying)
        atomObject.Raise(value);
}

And then just forcing the practice on our team to never call Raise() directly on an Atom.
Sucks, but works.

@soraphis
Copy link
Collaborator

Wouldn't this break the ability to raise AtomEvents (with parameters) from unity events in the inspector?

just pass null, or the atom itself.

@TheXRMonk
Copy link
Author

Wouldn't this break the ability to raise AtomEvents (with parameters) from unity events in the inspector?

just pass null, or the atom itself.

Yes that's my point. You can't raise an atom with a value AND pass the caller using UnityEvents in the inspector. Which is one lf the main selling points of the whole architecture.

@soraphis
Copy link
Collaborator

Which is one lf the main selling points of the whole architecture.

I would fight that statement any day!

You can't raise an atom with a value AND pass the caller using UnityEvents in the inspector

Why wouldn't that work? It's a custom Inspector like any other. so if the Raise method is modified to receive a second parameter the inspector will be modified obviously too.

E.g. here:

https://github.com/unity-atoms/unity-atoms/blob/master/Packages/Core/Editor/Editors/Events/AtomEventEditor.cs#L28

just adding either null or atomEvent as a second parameter.


Please note that everything I wrote is meant as notes for a future Pull Request, not as a solution you could just implement in on the consumer end of the library. (I thought that was clear, since the issue has the label enhancement)

@TheXRMonk
Copy link
Author

TheXRMonk commented Nov 29, 2022

Why wouldn't that work? It's a custom Inspector like any other. so if the Raise method is modified to receive a second parameter the inspector will be modified obviously too.

Unity Events arent custom Inspectors.
Take a button as an example. Add a listener to that in the inspector with a reference to your atomEvent and call Raise() on that.
The UnityEvent property inspector doesn't allow for more than one parameter, so any methods defined in the Atom class (or base classes) with more than 1 parameter, wont be selectable.
Being able to integrate with build-in UnityEvents (which is being used by -i dare to say- the majority of all third party libraries) is critical.
With your proposal, if I want yo raise an atom from a UnityEvent inspector, I am limited to either get caller context for debug purposes, or pass a value.

I will attach a screenshot tomorrow once I'm at work for clarification. :)

As for the main sellingpoint - I agree to disagree ;)

@TheXRMonk TheXRMonk reopened this Nov 29, 2022
@TheXRMonk
Copy link
Author

Sry for close - fat phone thumbs...

opens git issue requesting a feature to revert "close with comment" entries

@soraphis
Copy link
Collaborator

  1. You can use Unity Events.

image

See https://docs.unity3d.com/ScriptReference/Events.UnityEvent_2.html
the arguments must be passed from code, though.


If you insinst on the inspector functionality, you can use something like https://openupm.com/packages/com.solidalloy.extevents/


A part of unity atoms - and the scriptable object architecture as outlined by Ryan Hipple - is to NOT rely on UnityEvent, that's the whole point of AtomEvents. ( For reference this part of the talk: https://youtu.be/raQ3iHhE_Kk?t=1764 )

So why not just create a

[System.Serializable]
public struct StringEventWithArgs{
    public StringEventReference unityAtomsEvent;
    public string argument;
}

then you can modify the argument in the inspector as you wanted and you don't need to go through the UnityEvent to invoke a UnityAtomsEvent.

@TheXRMonk
Copy link
Author

TheXRMonk commented Nov 30, 2022

Wow we are going in circles.

In the video you linked he litterally goes on to call the gameevent scriptable object from a unityevent like I've explained all along;
image
@35:50

And you litterally just linked an alternative to my previous post (a replacement package to UnityEvent) which i already addressed as insufficient since mosy third party libraries uses UnityEvents.

Lastly

you don't need to go through the UnityEvent to invoke a UnityAtomsEvent.

I know - but we want to (read my previous reasons/watch the video you just linked.)

@TheXRMonk TheXRMonk reopened this Nov 30, 2022
@lgarczyn
Copy link

lgarczyn commented Feb 3, 2023

What about something like this

public class UnityEventDebug<T>: UnityEvent<T> {

    #if UNITY_EDITOR

    Object _parent = null;

    public void SetParent(Object parent) {
        _parent = parent;
    } 

    new void Invoke(T value) {
        
        for (int i = 0; i < GetPersistentEventCount(); i++) {
            string name = GetPersistentMethodName(i);
            Object target = GetPersistentTarget(i);
            if (typeof(UnityAtoms.IRaisable<T>).IsAssignableFrom(target.GetType()) && name == "RaiseFnName") {
                // call but with parent;
            } else {
                // call normally
            }
        }
    }
    #else
    public SetParent(Object parent) {}
    #endif
}

@TheOnlyMunk
Copy link

What about something like this

public class UnityEventDebug<T>: UnityEvent<T> {

    #if UNITY_EDITOR

    Object _parent = null;

    public void SetParent(Object parent) {
        _parent = parent;
    } 

    new void Invoke(T value) {
        
        for (int i = 0; i < GetPersistentEventCount(); i++) {
            string name = GetPersistentMethodName(i);
            Object target = GetPersistentTarget(i);
            if (typeof(UnityAtoms.IRaisable<T>).IsAssignableFrom(target.GetType()) && name == "RaiseFnName") {
                // call but with parent;
            } else {
                // call normally
            }
        }
    }
    #else
    public SetParent(Object parent) {}
    #endif
}

Appreciate the thought, but again, this wouldn't work with Unity's built-in components, unless I'm unaware of a way to inject this new type into components like the default button.

Maybe some way similar to how https://github.com/handzlikchris/Unity.TransformChangesDebugger.API works, but I have no idea how that works.

@lgarczyn
Copy link

lgarczyn commented Feb 3, 2023

You could try assembly patching, but it's a pile of worm I think few people would get into.

@TheOnlyMunk
Copy link

Yea, seems like Harmony is the way he did it. Guess I would have to patch every unityevent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants