Skip to content
This repository has been archived by the owner on Feb 28, 2024. It is now read-only.

Add extension methods to add local synchronized actions to buttons #95

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Banane9
Copy link
Contributor

@Banane9 Banane9 commented Mar 20, 2023

To help with all those buttons mods add to inspectors and what not.

Uses a bool field as a proxy to sync other people's presses to the person who created the button.

@XDelta XDelta added the enhancement New feature or request label Mar 20, 2023
@ljoonal
Copy link
Member

ljoonal commented Mar 21, 2023

IMO this is outside the scope of NML, and should be achieved via a library/mod. Generally speaking the less possible ways for NML to break there are, the better, and this adds quite a lot that'd requite bumping the major semver version as soon or if Neos ever does update again and those updates touch any of the code used here.

@ljoonal ljoonal requested a review from zkxs March 21, 2023 17:39
@ljoonal ljoonal added the .NET Pull requests that update .net code label Mar 21, 2023
@Banane9
Copy link
Contributor Author

Banane9 commented Mar 21, 2023

this adds quite a lot that'd requite bumping the major semver version as soon or if Neos ever does update again and those updates touch any of the code used here.

If a Neos update changes the code base so fundamentally that the basic behavior of Buttons or the UIBuilder gets changed, I doubt that NML wouldn't need an update too. And pulling it up would prevent mods from having to duplicate the code / make it easier to update it for all of them at once.
I for one have quite a few mods that would have benefitted from this kind of Button setup that isn't just usable by the user that spawned it.

@zkxs
Copy link
Collaborator

zkxs commented Mar 22, 2023

from @ljoonal:

IMO this is outside the scope of NML, and should be achieved via a library/mod

I seem to recall you having thoughts on VRChatUtilityKit and how the whole maintenance of that went... but I can't remember what you actually had to say... I'm thinking you're the one here with actual experience on how best to subdivide modloader vs library features, so I'll defer to you on this.


from @ljoonal:

this adds quite a lot that'd requite bumping the major semver version as soon or if Neos ever does update again and those updates touch any of the code used here.

You think? This code doesn't look like it's touching any spicy FrooxEngine internals to me.


from @Banane9:

If a Neos update changes the code base so fundamentally that the basic behavior of Buttons or the UIBuilder gets changed, I doubt that NML wouldn't need an update too.

I'm not sold on this argument... if Neos breaks NML we want to be able to get it operational again ASAP so there's definitely value in limiting the surface area of Neos that we touch. Which right now is the execution hook and the version spoofer. The other stuff (SplashChanger and AssemblyHider) is NML internals we could disable in a pinch for the sake of getting NML working again.

But anyways I think the likelihood of a Neos update breaking LocalButtonPressActionExtensions is pretty slim, so I'm probably rambling more than needed on this topic.

@ljoonal
Copy link
Member

ljoonal commented Mar 23, 2023

You think? This code doesn't look like it's touching any spicy FrooxEngine internals to me.

The methods take in arguments and return types that are from Neos' internals (Button, IButton, UIBuilder).
Thus, if those types receive breaking changes, our semver would need to be bumped as well if we're being strict about it.
I assume it's not something anyone would actually keep track off of though...

I'm thinking you're the one here with actual experience on how best to subdivide modloader vs library features

¯\_(ツ)_/¯
VRChatUtilityKit was used extensively by a few mods, but it had quite terrible error handling and was prone to breaking.
A better example would be knah's UIExpansionKit, which granted, did break sometimes too, but not nearly as much and was basically a requirement for vrc modding.

IMO it's always better if we can limit unrelated things breaking.
An issue with VRChatUtilityKit was for example that if say it's OnPlayerJoin or whatever hooked method was missing, it'd now mean that all the other completely unrelated functions might stop working as well OnPlayerLeft, SocialRankChange, UI code, etc.

A version of a mod can be marked as broken, but how would you handle if NML works, except for this functionality?
Would that version now have to be flagged as broken too, even if someone doesn't use any mods that use these utility functions, making them update NML for pretty much no reason?
Whereas if things are cleanly split into separate libraries, specific versions of them can just be flagged as broken and be updated, without needing to affect anything that doesn't use them.

The other stuff (SplashChanger...

Except specifically built it in a way that it will not fail the rest of the mod loader if anything in it errors, specifically to avoid depending on Neos internal specifics.
And as we don't expose that behavior to any mods, it breaking IMO is acceptable and does not mean that NML needs to be flagged as broken just due to it.

@EIA485
Copy link
Member

EIA485 commented Apr 24, 2023

imo we dont really need this wrapper since neos's api already exists and is easy to use

@Banane9
Copy link
Contributor Author

Banane9 commented May 14, 2023

imo we dont really need this wrapper since neos's api already exists and is easy to use

This just wraps the annoyance of having to set up the ValueField and ButtonToggle to make it work for anyone, making for more concise code.

@art0007i
Copy link
Member

I think make this a separate library mod. other mods can then depend on it. this is the best solution here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request .NET Pull requests that update .net code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants