Bug Description

Why are we actively preventing other mods from adding interactions with Embeddium?

I'm trying to do an equivalent to BetterBrightnessSlider for 1.21, which involves a mixin into SliderControl so I can expand the value range, and embeddium actively prevents me from doing so?

..are mixing into internal Embeddium class org.embeddedt.embeddium.api.options.control.SliderControl, which is no longer permitted

Reproduction Steps

.

Log File

.

Why are we actively preventing other mods from adding interactions with Embeddium?

Mod integrations through mixins are not permitted (unless your mod locks itself to one Embeddium version). However, mod integrations in general are welcome. The expectation is that either Embeddium can read the data from vanilla somehow (so you don't need a mixin), or an API can be added on Embeddium's side. There are far too many unnecessary mixins in 1.20.1, and they create a lot of support work for me if an update ever breaks them, or if they destabilize the mod. Mixins are not a stable way for mods to interact with one another.

In your case, it sounds like it would be enough for Embeddium to read the range of the vanilla brightness option for its brightness slider. Is that correct?

Well whatever you're doing currently doesn't work, because adding embeddium disables my functionality

So now I have a broken mod that I can't fix because I'm being actively bubblewrapped outside of my control

I have already suggested a possible fix on my side that is likely to address the issue. Can you point me to your mod's code so I can at least understand what you are trying to do?

As I said, cross-mod mixins are not a sustainable solution. They're quick for you to write, but they generally end up pushing extra work onto me when they inevitably break.

And arbitrarily requiring you to individually consider every single use case and implement an API change for them doesn't increase the work for you?

I'm not sure I understand the logic here

I modify the gamma SliderableValueSet by replacing it with an extended one that returns new values unclamped by vanilla, which works perfectly fine in vanilla

I would fix it on my side automagically, but it seems SliderableValueSet doesn't expose a min/max/interval. In future I may see if it is possible to extract those values by constructing a vanilla GUI and then introspecting it. For now this solution should be simple enough.

Use OptionGroupConstructionEvent to replace the SliderControl with your own instance, like so:

@SubscribeEvent
    private static void onOptionGroupConstruct(OptionGroupConstructionEvent event) {
        if(event.getId().matches(StandardOptions.Group.RENDERING)) {
            for(int i = 0; i < event.getOptions().size(); i++) {
                var option = event.getOptions().get(i);

                if (option.getId().matches(StandardOptions.Option.BRIGHTNESS)) {
                    event.getOptions().set(i, OptionImpl.createBuilder(int.class, MinecraftOptionsStorage.INSTANCE)
                            .setId(ResourceLocation.fromNamespaceAndPath("yourmod", "brightness"))
                            .setName(Component.translatable("options.gamma"))
                            .setTooltip(Component.translatable("sodium.options.brightness.tooltip"))
                            .setControl(opt -> new SliderControl(opt, -100, 1200, 1, ControlValueFormatter.brightness())) // you may wish to use a custom formatter
                            .setBinding((opts, value) -> opts.gamma().set(value * 0.01D), (opts) -> (int) (opts.gamma().get() / 0.01D))
                            .build());
                    break;
                }
            }
        }
    }

You may want to adjust the formatter, binding, interval, etc., but it should allow you to do everything the mixin could.

I'll investigate my options here

What happens when I need to mixin something niche/specific for a private circumstance that can't go in embeddium's public API?

Then you can use the provided escape hatch which is to set a dependency on a specific version of Embeddium. This guarantees your target class will not change under your feet in a future update, and prevents me from having to deliberately not change local variables and such in methods just to keep mixins working. (Yes, I had to do that numerous times in 1.20.1.)

With a version lock, the mod should allow you to mixin freely. If it does not, please bring that to my attention.

Very disappointing

I won't elaborate further, but this interception does pose pretty annoying problems for my use case

I can acknowledge the problem you have with people arbitrarily breaking stuff, but I'm not sure deciding no-one can play ball is the go-to approach here

Thanks for your response regardless

I'm not sure deciding no-one can play ball is the go-to approach here

This is a very uncharitable comment. Making the taint detector not check for the version lock, and just always ban mixins would be the equivalent of deciding no one can play ball. I did not do that; I explicitly provided the escape hatch for any use cases I could not support.

Anyways, your feedback will be taken into consideration, and I may decide to downgrade the check to a warning and just not provide support for any issues reported to me if mods targeting internals with mixins are installed. Not sure yet.

ays b

Right but you're entirely missing use cases in which people may want to keep embeddium updated, but not need to push a new version of their other mod every single time you do - particularly in environments where there may be more than one person working on something (such as a server group, content creation group, or private entertainment group)

Your presumption that pinning fixes it completely ignores the fact that not everyone is in the exact same circumstance that you assume is the default circumstance when you made that decision.
There are contexts and situations out there that your version pin workaround does not work well for - and actively contributes to and creates its own problems, which leaves it as an unviable solution.

What if someone is making a private server in which they hire someone to make a mod that fixes an incompatibility between embeddium and another mod (say, Valkyrian skies) - an incompatibility that only exists on this server because of the context and would otherwise be unsuitable or actively damaging to embeddium's public API.
If they then decide a month down the track they need to update embeddium, they now have a random unavoidable crash they very likely won't even understand, which then requires them to go and re-hire that person (who may not even be in the modding space anymore) to do a re-update of their mod to fix.

you're entirely missing use cases in which people may want to keep embeddium updated, but not need to push a new version of their other mod every single time you do

There is absolutely no guarantee their mod can continue to work if it touches internals, whether my taint detector explicitly crashes it or not. Internals are not intended for public use. There is always a potential that they will need to release an update.

You should realize that the main reason mixin mods are not breaking regularly on 1.20.1 is because I have made a concerted effort to minimize changes to areas of the code that are hotspots for mixins to target. It is not because mixins are stable (they are not). Maintaining mixin compatibility placed many constraints on what I can change for 1.20.1 and is not something that can be done indefinitely. It was intended as a stopgap until proper APIs were developed to replace the most common use cases. Many other addons/mods have migrated to APIs at this point.

Relying on internals of other software is simply not a good software development practice. This is not how production-ready software integrations are built in many other ecosystems.

There are contexts and situations out there that your version pin workaround does not work well for - and actively contributes to and creates its own problems, which leaves it as an unviable solution.

In which case, I have proposed the alternative solution above of going back to the warning, and simply not providing support to users that use mods with internal mixins - it will be on the developers of those mods to provide their own support. This will still not eliminate the actual issue of the internals having no stability guarantee, however.

What if someone is making a private server in which they hire someone to make a mod that fixes an incompatibility between embeddium and another mod (say, Valkyrian skies) - an incompatibility that only exists on this server because of the context and would otherwise be unsuitable or actively damaging to embeddium's public API.

It would be very unwise for them to commission a patch mod for another actively maintained mod and then not also be willing to retain that developer for future updates. As I mentioned above, you can't reasonably expect a mod outside your control to never change its internals.

Otherwise, it would make the most sense for that kind of server to maintain its own Embeddium fork and pull the critical fixes it needs (the license permits this), or not update Embeddium at all.

Obviously, this does not apply if the server is patching an Embeddium version that no longer receives updates (such as those for 1.19.2 & older) as the internals aren't going to be changing. 😉

In which case, I have proposed the alternative solution above of going back to the warning, and simply not providing support to users that use mods with internal mixins - it will be on the developers of those mods to provide their own support. This will still not eliminate the actual issue of the internals having no stability guarantee, however.

I would argue that not only is this acceptable - it's what the expectation should be
It was never your responsibility to support mods that broke your stuff via mixins, at any point.

It should be an explicit warning backed up with the statement that no support will be offered, with issue handling managed to that effect, not the current approach of no-one gets anything

Taint detector will be downgraded back to a warning in 1.0.7.