chark / unity-scriptable-objects

Game events and shared data realized using scriptable objects
MIT License
9 stars 1 forks source link

Split up & simplify library #30

Open Edvinas01 opened 3 years ago

Edvinas01 commented 3 years ago

I've used this library in multiple projects now, it seems that the most useful part is events. I honestly used MutableObject just a few times and its usefulness even then was questionable. If it turns out that the MutableObject functionality is needed, it can be moved to a separate library as not to bloat this one.

Given this, I think the following steps would make most sense (throughout multiple releases):

  1. Deprecate MutableObject functionality.
  2. Finish up improvements for GameEvent and GameArgumentEvent.
  3. Drop MutableObject functionality.
  4. Rename the repository to something something *Events, as the focus is only on events.
Edvinas01 commented 3 years ago

I think that GameEvent and GameArgumentEvent should be the same thing. This could be achieved where every event would require an arguments object, where simple GameEvent instances would receive Empty arguments object or something along the lines.

In a situation where each event would receive an object, we could store quite a lot of info in each event argument instance. This would be beneficial for: https://github.com/chark/unity-scriptable-objects/issues/27, https://github.com/chark/unity-scriptable-objects/issues/23 and https://github.com/chark/unity-scriptable-objects/issues/22.

One downside of this approach is that performance might tank, but in contrast it would make the API a lot more flexible.

KacperKenjiLesniak commented 3 years ago

@Edvinas01 Just thought I might share my thoughts as I'm currently using this library a lot 😄

I have also used this library in a couple of projects now and every time I made strong use of the MutableObject functionality, so from my perspective, it would be great to keep it. I know it is more of a wrapper to an existing functionality than a real standalone feature, but it is very handy nonetheless.

As for the second comment, I agree it would make sense to have GameEvent be a special case of an empty GameArgumentEvent and if the Empty argument is not instantiated every time, but just a static shared object like Enum I don't see why it would impact performance.

Another thing is adding additional info to each event argument instance, which might be heavy, so it would be great to have a possibility of switching between a debug mode, where this info is passed along each call as well as a production mode where the arguments are stripped to a minimum.

Edvinas01 commented 3 years ago

@KacperKenjiLesniak Its awesome to hear that you're using it! I'm planning on plugging it into my thesis project as well :D

I have also used this library in a couple of projects now and every time I made strong use of the MutableObject functionality, so from my perspective, it would be great to keep it. I know it is more of a wrapper to an existing functionality than a real standalone feature, but it is very handy nonetheless.

I think your argument is sound. I was thinking of how this could be tackled without screwing everyone who relies on this package (even if the number of users is tiny). I'm thinking the best approach right now is to make 2 new packages/repositories - one for the events (almost ready) and one for the mutable objects, then they could be linked in the README.md of this package as a "redirect" for new users.

I feel that combining them doesn't make much sense as they do different things. Also its hard to give a fitting name when keeping them both in the same place :x

Also I'm curious, did you find the ResetType functionality useful?

As for the second comment, I agree it would make sense to have GameEvent be a special case of an empty GameArgumentEvent and if the Empty argument is not instantiated every time, but just a static shared object like Enum I don't see why it would impact performance.

Currently I've setup something along those lines (BaseScriptableEvent is essentially GameArgumentEvent, I decided that this is a more fitting name as someone working on a non-game project might make use of this package):

namespace ScriptableEvents.Simple
{
    [CreateAssetMenu(
        fileName = "SimpleScriptableEvent",
        menuName = "Scriptable Events/Simple Scriptable Event",
        order = -10
    )]
    public class SimpleScriptableEvent : BaseScriptableEvent<SimpleArg>
    {
        /// <summary>
        /// Raise this event without an argument.
        /// </summary>
        public void Raise()
        {
            Raise(SimpleArg.Instance);
        }
    }
}

And the SimpleArg class looks like this:

namespace ScriptableEvents.Simple
{
    public class SimpleArg
    {
        public static readonly SimpleArg Instance = new SimpleArg();

        private SimpleArg()
        {
        }
    }
}

Another thing is adding additional info to each event argument instance, which might be heavy, so it would be great to have a possibility of switching between a debug mode, where this info is passed along each call as well as a production mode where the arguments are stripped to a minimum.

I was hoping that the debug feature could be used to enable/disable this, but currently I'm having a hard time figuring out how the API should look if I wanted to pass around an "event data object". The problem with using an "event data object" is that it makes the API a lot less flexible, it essentially removes the ability to slot in events into Unity UI components such as sliders, etc - the UnityEvent API is not that flexible. Also it generates a lot of garbage. I think this will need more thought and/or some stack-trace hacking, but I don't think that this is a must have feature for now.

Edvinas01 commented 3 years ago

The new "game event" package can be found here: https://github.com/chark/scriptable-events

With a slim change-log for the upcoming release: https://github.com/chark/scriptable-events/blob/master/Packages/com.chark.scriptable-events/CHANGELOG.md

Right now its missing automation and its "first" release.

Edvinas01 commented 3 years ago

The first release (second with a hotfix) is up and running at https://github.com/chark/scriptable-events. I've also registered the package on OpenUPM as well for ease of use.

I've closed existing issues on this repository and migrated relevant ones to the scripable-events one. I'll keep this issue open until the MutableObject functionality is migrated. Will comment here after that is done and update README.md.