-
Notifications
You must be signed in to change notification settings - Fork 18
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
Enable toggling transform controls #578
base: main
Are you sure you want to change the base?
Conversation
Preview PR at appsharing.space |
Integration tests report: appsharing.space |
Thanks! |
this.setState( | ||
old => ({ ...old, transform: transformEnabled }), | ||
() => { | ||
this._updateSelected({}); |
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.
Is this right? Shouldn't we just set the transformControls object's visible
attribute here instead? Or the enabled
property
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.
yes, i tried that but just setting those was preventing transformControls from disabling or enabling right on clicking the toggle button, which is why I also had to trigger this part of code:
JupyterCAD/packages/base/src/3dview/mainview.tsx
Lines 1181 to 1196 in 3340d1d
this._transformControls.attach(matchingChild as BasicMesh); | |
const obj = this._model.sharedModel.getObjectByName(selectedMeshName); | |
const positionArray = obj?.parameters?.Placement?.Position; | |
if (positionArray && positionArray.length === 3) { | |
const positionVector = new THREE.Vector3( | |
positionArray[0], | |
positionArray[1], | |
positionArray[2] | |
); | |
this._transformControls.position.copy(positionVector); | |
} | |
this._transformControls.visible = true; | |
this._transformControls.enabled = true; |
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.
Hmm, though calling _updateSelected({})
means unselecting anything that was previously selected.
I'm a bit confused you need this code to be triggered again. This code should already have been triggered upon the latest selection.
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 code should already have been triggered upon the latest selection.
i am needed to reselect the object manually after toggling the transform controls from the button
though calling _updateSelected({}) means unselecting anything that was previously selected.
yes this is not ideal and should be done better, happy to start iterating & hear thoughts
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 am needed to reselect the object manually
Do you know why you need that?
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 am needed to reselect the object manually
Do you know why you need that?
It was most probably needed because of missing attach & detach calls.
Is it better after f0b2c6d
? if not what should be the right method to get the selected object?
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.
That does not look correct to me this._pick()
will run the raycast picking, so it's entirely dependent on your current mouse position. This means your code does not work (won't attach to the correct object) if the mouse is mal-positioned.
I'm still not sure why you need to attach/detach here?
It should already have been attached here on the latest selection. So AFAIU your code here should only do enabled = false/true
and visible = false/true
.
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.
Just have a comment/question
Yes, that looks great. Thank You 🚀 i'll be on it |
This element can also be used for the clipping plane |
toggle-transform.mov
Icon is a draft
In the direction of #573