Yellow-Dog-Man / Resonite-Issues

Issue repository for Resonite.
https://resonite.com
131 stars 2 forks source link

Plugins that add new setting components break when trying to generate SyncMethod Action buttons #2538

Open BlueCyro opened 1 month ago

BlueCyro commented 1 month ago

Describe the bug?

When creating a new setting class via a plugin and making a setting action property, the engine will reject it as an invalid type when the settings dialog tries to make the button for it.

To Reproduce

1) Load this plugin (dll found in the ReleaseBuild folder) 2) Navigate to the "Misc" tab of the settings menu 3) Notice the exception that appears in your logs about the action proxy being an invalid data model type when you press the "Test method" button

Expected behavior

The settings menu should allow creation of this delegate and not throw the exception

Screenshots

No response

Resonite Version Number

2024.7.11.1293

What Platforms does this occur on?

Windows

What headset if any do you use?

Desktop

Log Files

DESKTOP-6C2UJFB - 2024.7.11.1165 - 2024-07-11 16_32_30.log

Additional Context

I've verified that the DLL is marked with the proper DataModelAssembly(DataModelAssemblyType.Optional) attribute for optional assemblies. Also note that the way I've registered the setting with the settings menu is very dumb, but shouldn't be influencing the fact that the engine sees the delegate as invalid. I just needed a quick way to register it for this purpose of this issue.

Reporters

Myself

shiftyscales commented 1 month ago

Seeking input from @Frooxius.

Nytra commented 1 month ago

I have been able to get plugin setting action buttons to work. My plugin uses DataModelAssembly(DataModelAssemblyType.Core) though which may be significant.

Frooxius commented 1 month ago

Is TestPlugin.TestGuy marked as data model type?

Nytra commented 1 month ago

It's a SettingComponent<S> so it should be automatically

BlueCyro commented 1 month ago

Is TestPlugin.TestGuy marked as data model type?

The assembly is marked with the data model assembly atteibute, however the class itself isn't marked with a data model type attribute. But as nytra mentioned it's derived from the setting component class so it should be automatically right?

Frooxius commented 1 month ago

That's not how that works. Anything that you want to use as a generic argument in the data model needs to be marked explicitly as well.

This is to prevent from sneaking in "wrong" types through generics.

You can launch Resonite with -ValidateTypes argument, which will scan everything and notify you of which types are being used that haven't been properly marked.

Nytra commented 1 month ago

I used the -ValidateTypes argument with the plugin above and it said there were no invalid types.

Nytra commented 1 month ago

@BlueCyro The button works if you use DataModelAssembly(DataModelAssemblyType.Core) or DataModelAssembly(DataModelAssemblyType.UserspaceCore)

I think if you use DataModelAssembly(DataModelAssemblyType.Optional) the type TestPlugin.TestGuy is not allowed in any world, which is why the button doesn't work (It needs to attach a SettingActionProxy<TestGuy> component in the settings facet)

However if that is the case, it's odd that the setting component gets attached in the first place...

Frooxius commented 1 month ago

@Nytra Can you point me to how exactly is this type used? It's probably more dynamic, so the validation doesn't find the usage of it.

Also yes. Optional data model assemblies are not included in the allowed set by default, you'd have to patch code to allow that.

If you want to use it as a setting, then add it into UserspaceCore.

Nytra commented 1 month ago

@Frooxius

This is the entire code of the plugin. The definition of the setting component looks normal to me. The way the setting component gets registered is inside of a ImplementableClass connector which is not the usual way it would be done. Could this be throwing off the detection?

using FrooxEngine;
using Elements.Core;

namespace TestPlugin;

[SettingCategory("Misc")]
public class TestGuy : SettingComponent<TestGuy>
{
    Sync<string> HUHHH;

    [SettingProperty("Test method")]
    [SyncMethod(typeof(Action))]
    public void TestMethod()
    {
        UniLog.Log("I'm a test!!!!!!!!!!!!!!!!!");
    }
}

[ImplementableClass(true)]
public static class SettingShim
{
    private static Type? __connectorType;
    private static Type? __connectorTypes;
    private static HackConnector InstantiateConnector() => new();

    // This is really just a terribly awful way to inject in the settings menu, I just needed something quick
    static SettingShim()
    {
        UniLog.Log($"Shimming {nameof(TestGuy)} into the settings menu");
        Settings.GetActiveSetting<TestGuy>();
    }

    private sealed class HackConnector : IConnector
    {
    public IImplementable? Owner { get; private set; }
    public void ApplyChanges() { }
    public void AssignOwner(IImplementable owner) => Owner = owner;
    public void Destroy(bool destroyingWorld) { }
    public void Initialize() { }
    public void RemoveOwner() => Owner = null;
    }
}

Images of plugin effects

The setting component gets created in the Userspace like normal:

Screenshot 2024-07-18 173532

It adds a button to the bottom of the Misc category:

2024-07-18 16 34 02

It creates a SettingActionProxy<TestGuy> on the button.

2024-07-18 16 34 53

Frooxius commented 1 month ago

Ah I see what the problem is now. TestGuy is marked implicitly, since it derives from a component.

However since you're making the assembly as "Optional", it isn't allowed by default in the data model.

You'll need to either mark it as Core or UserspaceCore or you'll need to inject code that allows this assembly in Userspace (though it's much simpler to just mark it as UserspaceCore, since that has the same result).

Nytra commented 1 month ago

What I'm a bit confused about though is why the component gets attached in the userspace if it's not allowed? Is it bypassing the checks somehow?

Frooxius commented 1 month ago

Yes, the setting system forces it through since that exists "outside" of the data model.

shiftyscales commented 1 month ago

Have you had your questions answered sufficiently / can this issue be considered as resolved, @BlueCyro?

Frooxius commented 1 month ago

We should probably add validation there to catch this kind of issue earlier.

shiftyscales commented 1 month ago

Sounds good. :)

BlueCyro commented 1 month ago

Have you had your questions answered sufficiently / can this issue be considered as resolved, @BlueCyro?

Yup, it makes sense to me. I'll probably close this up once validation is added just so froox's point isn't lost