Skip to content
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

Simplify Gizmo creation #6981

Closed
kungfooman opened this issue Sep 22, 2024 · 7 comments · Fixed by #6992
Closed

Simplify Gizmo creation #6981

kungfooman opened this issue Sep 22, 2024 · 7 comments · Fixed by #6992
Assignees

Comments

@kungfooman
Copy link
Collaborator

Currently we need this code to create a gizmo:

const gizmoLayer = new pc.Layer({
    name: 'Gizmo',
    clearDepthBuffer: true,
    opaqueSortMode: pc.SORTMODE_NONE,
    transparentSortMode: pc.SORTMODE_NONE
});
const layers = app.scene.layers;
layers.push(gizmoLayer);
camera.camera.layers = camera.camera.layers.concat(gizmoLayer.id);

// create gizmo
const gizmo = new pc.TranslateGizmo(app, camera.camera, gizmoLayer);
gizmo.attach([box]);

The upper/big block of code is just... complicated boilerplate code that no one can remember (at least not me). The last two lines do what a person really wants from a gizmo.

Ideally I would like the required code for adding a gizmo to be short like this:

const gizmo = new pc.TranslateGizmo(app, camera.camera);
gizmo.attach(box); // Accept non-arrays, just like e.g. `HTMLElement#append`

It would be nice if the gizmo system ensures creation of the layer if it doesn't exist yet?

Considering we are also running into issues like #6949 it's nice to not make everyone refactor layer code if it happens to need a change. The gizmo system would just do what is needed for certain updates.

@willeastcott
Copy link
Contributor

I have to say, I did think the same the first time I played with the Gizmo API. Let's get some thoughts from Chief Gizmo Architect @kpal81xd...

@mvaligursky
Copy link
Contributor

I would suggest for the constructor to take an optional layer argument:

new TranslateGizmo(app, camera.camera, layer = null);

and if this is not provided, the gizmo should use the existing Immediate layer. If the user needs to have it in a separate layer, they can create one using the Editor GUI for example and avoid layer creation using the script.

@kpal81xd
Copy link
Contributor

So the first version of the api included the layer creation itself however we felt like it is clearer for the user to provide and setup their own layer in case of a particular behavior.

Including the layer also comes with complications of layer insertion being dependent on when the gizmo is created along with potential naming conflicts.

One solution might be allowing for gizmo creation with an optional parameter for settinga custom layer?

@Maksims
Copy link
Contributor

Maksims commented Sep 23, 2024

Also, if you have to provide CameraComponent, you can get an app from it instead of explicitly providing it. Less arguments - better.

@kungfooman
Copy link
Collaborator Author

So the first version of the api included the layer creation itself however we felt like it is clearer for the user to provide and setup their own layer in case of a particular behavior.

I would just like it configurable but still opinionated enough to basically never really needing to configure it besides in cases which are clearly "non-defaultish".

and if this is not provided, the gizmo should use the existing Immediate layer. If the user needs to have it in a separate layer, they can create one using the Editor GUI for example and avoid layer creation using the script.

I believe putting it by default on Immediate is contra productive - it would depth test and be drawn e.g. under a box that is supposed to be "movable"? People may not find that gizmo.

@kpal81xd
Copy link
Contributor

Having thought about it a little more I dont think I would like to set the gizmo default to create the layer internally. I believe there should be a minimum expected understanding to use gizmos and having better visibility over your layer to know how it behaves is better than having the default create the layer internally.

@kungfooman
Copy link
Collaborator Author

Playing devil's advocate again, give it a rest...

Same story as with the Gizmo Attach issue, other Gizmo implementations are just simple and straight-forward to use, but we need to collect half-baked idiosyncrasies again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants