-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Gizmo fixes #6763
Gizmo fixes #6763
Conversation
@@ -51,7 +51,7 @@ const SHADER = { | |||
// store z/w for later use in fragment shader | |||
vZW = gl_Position.zw; | |||
// disable depth clipping | |||
gl_Position.z = 0.0; | |||
// gl_Position.z = 0.0; |
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.
Delete?
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.
It looks cleaner when clipping with this so would prefer to have it in but the selection area and rendered area do not align
@@ -122,7 +122,8 @@ data.set('camera', { | |||
}); | |||
const camera = new pc.Entity('camera'); | |||
camera.addComponent('camera', { | |||
clearColor: new pc.Color(0.1, 0.1, 0.1) | |||
clearColor: new pc.Color(0.1, 0.1, 0.1), | |||
farClip: 1000 |
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 won't the example work with the default far clip?
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.
It does it you can just see the the axis lines clipping when zooming out
@@ -264,28 +265,40 @@ class RotateGizmo extends TransformGizmo { | |||
return this._shapes.x.tolerance; | |||
} | |||
|
|||
/** | |||
* @param {string} prop - The property. | |||
* @param {any} value - The value. |
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.
Isn't the type here number
?
@@ -315,40 +315,67 @@ class ScaleGizmo extends TransformGizmo { | |||
return this._shapes.xyz.tolerance; | |||
} | |||
|
|||
/** | |||
* @param {string} prop - The property name. | |||
* @param {any} value - The property value. |
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.
number
?
_setArrowProp(prop, value) { | ||
this._shapes.x[prop] = value; | ||
this._shapes.y[prop] = value; | ||
this._shapes.z[prop] = value; | ||
} | ||
|
||
/** | ||
* @param {string} prop - The property name. | ||
* @param {any} value - The property value. |
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.
number
?
_colorSemi(color) { | ||
const clone = color.clone(); | ||
clone.a = this._colorAlpha; | ||
return clone; | ||
} | ||
|
||
/** | ||
* @param {string} axis - The axis to update. | ||
* @param {any} value - The value to set. |
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.
Color
?
@@ -305,18 +311,31 @@ class TranslateGizmo extends TransformGizmo { | |||
return this._shapes.face.tolerance; | |||
} | |||
|
|||
/** | |||
* @param {string} prop - The property to set. | |||
* @param {any} value - The value to set. |
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.
OK, I'm gonna stop pointing these out now....maybe there's a good reason it's any
! 😄
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.
All of these are any because its a generic property update so the property could technically be any type
* disabled depth clipping and set start selection to be near clip instead of 1 * fixed gizmo scaling based on fov and aspect ratio * removed aspect ratio scaling * use distance instead of length for scale * removed unused property * removed redundant funciton for gizmo creation * refactor variable names * gizmo jsdoc and format cleanup * hoisted out guide angle color into constant * kept one pointDelta instance per class
* Gizmo fixes (#6763) * disabled depth clipping and set start selection to be near clip instead of 1 * fixed gizmo scaling based on fov and aspect ratio * removed aspect ratio scaling * use distance instead of length for scale * removed unused property * removed redundant funciton for gizmo creation * refactor variable names * gizmo jsdoc and format cleanup * hoisted out guide angle color into constant * kept one pointDelta instance per class * fixed gizmo scaling based on fov and aspect ratio * removed aspect ratio scaling * use distance instead of length for scale * removed unused property
Fixes #6671
Fixes #6695