-
-
Notifications
You must be signed in to change notification settings - Fork 938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Expose and cleanup editor gizmos #2127
Conversation
Ohh, does it mean, I can access it through code only and add to any entity as it is just a regular component? |
@VaclavElias It's not really a component, it only inherits from the |
Thanks for the hints as usually. I will try it. Yes, it could be useful to enable/disable it through running game as I found it still very useful even now, to orient in the 3D, when debugging/testing running game. Also, if I may suggest. Adding optionally text X,Y,Z would be also helpful. |
# Conflicts: # sources/editor/Stride.Assets.Presentation/StrideDefaultAssetsPlugin.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of remarks reading the code. I'll try it locally next before approving.
/// <summary> | ||
/// Contains the id of a component | ||
/// </summary> | ||
public readonly ref struct OpaqueComponentId |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll quickly cover the way picking works just to make sure we're on the same page;
Scene picking renders models and reads the component id of the one under the mouse cursor, so, those ids come from the rendering components, not the component the gizmo is observing.
The gizmo can freely create any kind of component or mesh, so we don't know in advance which model component map to which gizmos. We have to ask the gizmo whether that id comes from them.
This logic lives in user-land, and we can't just give the user a random int and expect him to figure it out, so this thin api over the int should be more intuitive as to how users should work with that particular part of the gizmo api.
This is one of the weaker area of the api I feel, so if you can think of something that is more intuitive than this, feel free to let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for clarifying. As long as the documentation mentions that, it's fine for me. It is a simple solution and probably good enough for now. We can refine later if more usage scenarios appear.
} | ||
catch (MissingMethodException e) | ||
{ | ||
throw new MissingMethodException($"{nameof(IEntityGizmo)} '{gizmoType}' must have a constructor with exactly one parameter of type '{component.GetType()}'", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was the default exception message not satisfying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would output System.MissingMethodException: Constructor on type 'Gizmo' not found.
, it is a bit misleading since you may very well have a constructor on that type, but we require a constructor with exactly one argument of a specific type.
Just making things clearer for our users.
Looks like it can only work for a user gizmo if |
@Kryptos-FR it is not, I have two entities here, one with the component as first, and the other one as second, both show up with |
@Eideren looks like it is working now. Maybe I needed to restart the editor. LGTM. |
Thanks for reviewing @Kryptos-FR |
PR Details
This PR provides support for user-defined gizmos, most of the logic was already there, I just had to clean out the editor only stuff from the api.
Here's a simple example:
Types of changes
Checklist