nike4613 / BeatSaber-IPA-Reloaded

A Unity mod injector built for Beat Saber.
https://nike4613.github.io/BeatSaber-IPA-Reloaded/
Other
185 stars 28 forks source link

A proposal to change the way that plugin enabling and disabling is done #30

Closed nike4613 closed 4 years ago

nike4613 commented 4 years ago

In the in-development beta of BSIPA 4, all BSIPA plugins have been made to be, by default, enableable at runtime, however not disableable at runtime. This is a strange inconsistency and has several ways of being fixed:

  1. Make the default be both enableable and disableable at runtime
  2. Leave it as is
  3. Swap the inconsistency to make it by default disableable but not enableable at runtime
  4. Make the default be neither enableable nor disableable at runtime, so that that is opt-in
  5. Completely redo the plugin interface so that both cases are explicit

My preference is, in order, 1, then 5, then 2.

For number 1: I believe that having the default allow runtime enabling and disabling would encourage mod authors to write more robust code, as it would need to be capable of withstanding multiple disable/enable cycles. It would also give users that wish to exercise the control that this system provides the ability to with more mods out-of-the-box.

Number 2 should be fairly self-explanatory, as its basically what already exists in BSIPA 3.

Number 5 is the most interesting one, and possibly the most promising. The way I currently envision such a change going would be such that the resulting plugin classes would look something like this:

For a non-enableable non-disableable plugin:

[BSIPAPlugin(PluginOptions.RuntimeStateStatic)]
public class Plugin 
{
    private PluginConfig config;
    [Init]
    public void Init(Config cfg)
    {
        config = cfg.Generated<PluginConfig>();
    }

    [OnEnable]
    public void OnEnable() 
    {
        // do your stuff for the enable event
    }

    [OnDisable]
    public void OnExit()
    {
        // do your stuff for game exit
    }
}

For an enableable and disableable plugin:

[BSIPAPlugin(PluginOptions.RuntimeStateChanging)]
public class Plugin 
{
    private PluginConfig config;
    [Init]
    public void Init(Config cfg)
    {
        config = cfg.Generated<PluginConfig>();
    }

    [OnEnable]
    public void OnEnable() 
    {
        // do your stuff for the enable event
    }

    [OnDisable]
    public void OnDisable()
    {
        // do your stuff for the disable event
    }
}

None of this is final, and any suggestions for it are welcome.

Kylemc1413 commented 4 years ago

5 or 4, followed by 2, with 1 and 3 being horrible ideas imo

Auros commented 4 years ago

In my opinion, the best option is 1. A large part of the controversy for this and most large changes to the community is that it more work for modders and that its a pain. As modders and the people keeping the community alive we should be willing to make these changes. Our code should be more robust to allow these changes. It gives the user more choice and can make developing mods easier for the people who utilize it. Another issue I've seen brought up is the lack of documentation which will increase the hurdles new modders have to go over. With the release of BSIPA 4, it's getting all new documentation and the wiki is being rewritten. The whole argument of "we shouldn't change it" is due to the unwillingness for some people to change their mods, and for those people, they can always simply change a true to a false.

sc2ad commented 4 years ago

I like 5, and then 1, with 2 being suboptimal, personally, although not sure how much opinion counts.

opl- commented 4 years ago

Considering the complexity disabling and even enabling at runtime might add, the choice should be left to the modder. The framework should support both cases, but not enforce any of them. Not all mods are worth the effort that would be required to support this and forcing people to implement it can, in my opinion, only negatively impact the modding community. For this reason I'm in favor of 5 or 4.

nike4613 commented 4 years ago

The final form resembles Option 5 the most and is now merged into master. See the docs for usage.