Yellow-Dog-Man / Resonite-Issues

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

Allow viewing all delegates in inspector panels. #2367

Open art0007i opened 3 months ago

art0007i commented 3 months ago

Is your feature request related to a problem? Please describe.

The FrooxEngine has these delegates it likes to use in a code generated ui (inspectors etc.). The easiest example I can think of is RefEditor which has a delegate for clicking in a reference, but there are many other cases where delegates are used to improve functionality.

Currently the only way to get these delegates is to "find them in the wild" which means to make the ui generate normally and then rip out the specific components that have the delegate to use it yourself, but this process can be quite annoying and can't always achieve everything you might want to.

Describe the solution you'd like

A way to view all valid delegates (ones marked with the SyncMethod attribute) instead of just a very limited selection of them like there is currently.

Describe alternatives you've considered

Use the mod https://github.com/art0007i/ShowDelegates the mod also puts all the delegates in a expandable element which is collapsed by default to prevent clutter in the inspector.

Additional Context

I'm making this request because I've seen my mod recommended to other people a few times recently, so I think it would be a really good addition to have without any mods.

Requesters

No response

JackTheFoxOtter commented 3 months ago

I would very much appreciate that. I've run into this a couple of times where I had to ask people with the mod installed to grab the delegate I needed for me. Also I think the mod fixes a bug where method proxy nodes for some delegates with arguments don't generate properly. (Not sure if that one is related, it might be fixed implicitly by how the mod exposes delegates)

art0007i commented 3 months ago

It's hard to say whether it's a bug with protoflux method proxies not generating, but I did have to explicitly fix it using the mod.

It happens because of delegate types. in C# there are Action<T>, Action<T1,T2> etc. and Func<TResult>, Func<T,TResult>, Func<T1,T2,TResult> etc. and protoflux can only spawn these "Action" and "Func" delegates, while FrooxEngine uses some custom delegates such as "ButtonEventHandler", the thing is that every delegate can be represented using either Action or Func for example: ButtonEventHandler is equivalent to Action<IButton, ButtonEventData>

my mod adds a bit of code to the protoflux tool to perform this conversion from custom delegate type to one of the generic ones

shiftyscales commented 3 months ago

As a reminder- if it is believed that bugs exist in the unmodified client, please ensure there is also an issue posted on this repository as well detailing the specifics of the issue.

shiftyscales commented 3 months ago

Pre-empting a response from @Frooxius or @ProbablePrime - but I assume that the "very limited selection of them like there is currently" are exposed because it is safe to do so / they were previously requested to be exposed.

So on a related note- are there any specific ones you would be wanting to see exposed in paticular, @art0007i @JackTheFoxOtter?

It would probably be better to focus this issue on the particular ones you want exposed (and the workflows / reasons for why you want them to be so) rather than just exposing them all by default.

JackTheFoxOtter commented 3 months ago

The main issue here is that the delegates do exist, and you can already access and use them in most cases, it's just really bothersome to do so without the mod. We could make issues to request to expose some of them, but that would create a bunch of work, and likely would still encourage people to use the mod to actually be able to access all features of the components.

Generally what I like about Resonite is that it's not really imposing any arbitrary limitations to creators without a good reason. Only showing some of the delegates but not all is kind of an arbitrary limitation, and if there is a good reason for it, I'd like it clarified.

Exposing them wouldn't really be unsafe, it would just make it easier for creators to use them. The one potential issue I can see here is maintaining backwards compatibility. I assume some of the delegates are intended to be used only internally, and might change if the components change in the future, so the internal functionality might not stay compatible. But even then, that's kind of the same as the internal properties of components denoted by an underscore. They are still exposed so creators can interface with them, even if that's not encouraged and might break. But in that case I much prefer educating creators and letting them make the decision wether or not to use those on their own.

shiftyscales commented 3 months ago

I didn't mean safe in a security sense- but in a content building sense, @JackTheFoxOtter.

The one potential issue I can see here is maintaining backwards compatibility.

This would be why.

JackTheFoxOtter commented 3 months ago

I didn't mean safe in a security sense- but in a content building sense, @JackTheFoxOtter.

The one potential issue I can see here is maintaining backwards compatibility.

This would be why.

If that's the reason I'd still like it to be clarified / somehow communicated to creators considering that people are already using many of those today.

shiftyscales commented 3 months ago

somehow communicated to creators

We have.

https://wiki.resonite.com/index.php?title=Things_to_Avoid#Working_around_unsupported_or_not_yet_implemented_features

Certain features that are yet to be implemented officially can be "hacked in" or achieved through modifying certain internal systems and properties. While we're ok with experimenting and toying around, please be aware that those approaches will very likely break, without guaranteed prior warning and official implementation in place.

This can leave you with broken content without ability to fix it until a much later point in the future, so we recommend avoiding such approaches in the first place and instead finding different method using already supported features.

You can find list of planned features on our official GitHub: https://github.com/Yellow-Dog-Man/Resonite-Issues and vote on them so we prioritize them sooner. For any official features we always put great care into ensuring long term content compatibility over faster implementation, to ensure that future updates will keep your content functional.

If we close a certain issue as something that we don't plan on implementing, we strongly advise against using hacks and workarounds to achieve that result completely, as those will be brittle and will break without replacement coming at all. In such cases you'll have to find a different approach that works with the design of the system to ensure long term compatibility.

Frooxius commented 3 months ago

The inspector already does this - any explicitly marked SyncMethods will be shown in the inspector automatically.

art0007i commented 3 months ago

The inspector already does this - any explicitly marked SyncMethods will be shown in the inspector automatically.

not unless it's a private method or it's SyncMethod type is "Delegate" (which most are)

art0007i commented 3 months ago

well in my case I simply wanted to have an easier time making reference fields that can be clicked into. most people don't use this in their ui and it's because the accessibility to the delegate is limited to stealing it from inspector panels. this is one case that I run into the most myself, but I've seen many more which are similar in nature.

there are basically 2 options and none are perfect: 1) show all delegates, which solves all simple cases but also there are a bunch of delegates that are less than ideal to have shown. 2) show the specific delegates I need here, which is not ideal because I can't list every delegate I might need in the future

Frooxius commented 3 months ago

The inspector already does this - any explicitly marked SyncMethods will be shown in the inspector automatically.

not unless it's a private method or it's SyncMethod type is "Delegate" (which most are)

Yes, because those aren't explicitly marked - they are implicit.

Those aren't necessarily safe to use arbitrarily - they might change at any time without warning and we might add limits on when they get called. If you use those, you do that at your own risk.

If we exposed those natively, then we'd have to take on the burden of maintaining that and that's not something we want to do.

art0007i commented 3 months ago

I think it would be good if the implicit delegates are shown with a warning or hidden by default or some other thing that makes it obvious that you use those at your own risk and they might break in the future.

shiftyscales commented 3 months ago

That is implicit with the use of mods that add unofficial / unsupported features, as highlighted above, @art0007i. People are ultimately going to do whatever they're going to do no matter what we warn against- and as shown above- we already make it clear in documentation that such practices are not supported, and have the possibility of breaking without forewarning in future.

That is the nature of doing anything that's not supported / not an official system.

Frooxius commented 3 months ago

Question here is - why do you want these to be exposed? I think showing them, even with warning, could make people use and rely on them more, despite the warnings.

In order to support this, we'd essentially see what benefit is there to implementing and exposing something that we generally don't want people to use in the first place - it would outweigh that (and just leaving it to mods).

In individual cases, we can evaluate any delegates and see if they could get exposed if we feel they can be supported long term.

art0007i commented 3 months ago

I think there are a lot of individual cases and while I could list all the ones I've ran into it wouldn't be an exhaustive list and exposing all would basically be a catch all solution

the biggest individual case by far is RefEditor, which I think should have all of it's implicit delegates marked as explicit

Frooxius commented 3 months ago

Unfortunately this is not one of the things where we could do catch all - exposing everything is not a good idea given the reasons I've given above.

Even looking at RefEditor, I don't see any delegates that we'd be comfortable exposing. All of them are its own internal functionality that are specific to it and the buttons it generates and absolutely should not be exposed.

This is making me feel like this is an XY kind of solution and that feature is being abused for something it's not meant for - the exact thing we warn again.

I'd recommend looking at why do you want the functionality of the RefEditor exposed? What are the things you're trying to use it for? Look at those underlying reasons and use-cases, perhaps there's requests there where we can provide generalized solutions to just do those things directly.

I can tell you for sure though - if you do depend on RefEditor and these - your stuff WILL break. This is likely going to go poof with inspector rework.

art0007i commented 3 months ago

I'm trying to make a reference field in my own ui, without having compromised functionality like not being able to click the value in. it would be nicer if it's possible to build a reference field like that from scratch instead of having to rip it from an inspector and restyle everything.

Anyway if RefEditor is going away how is one supposed to build a ReferenceField like that. I thought taking aparat an inspector panel would be a good way to learn how to make one but apparently it's not. Is there a list of components which are ok to use and won't break in the future that I can refer to?