-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Change private members in gizmo to protected #12796
Conversation
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12796/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/12796/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/12796/merge#BCU1XR#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.
LGTM
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 have commented on the issue create and have one comment regarding this specific PR -
is there a reason to make the private static function protected?
Not really functions, but static shader code. these 2 lines are in the whole 200+ line constructor function. maybe it is useful to overwrite the shader for convenience?
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12796/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/12796/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/12796/merge#BCU1XR#0 |
Hey @braineo, I think we are good to go Wanna remove the draft? (I can do it if you prefer :)) |
Oh there are other gizmo interfaces not added, I’m bit busy this week, gonna add all when I find some time. |
@braineo, no problem I guess another PR would do for the other ones |
Change private members in gizmo to protected Former-commit-id: 9938106a358e7b1f8c1e99a992cecea95264ec62
close #12795
I changed most of member from private to protected, even for those with public setters and getters, as setters and getters can be overwritten.
Except for
_tmpQuaternion
One thing I'm not sure about is the shader code in
planeRotationGizmo.ts
. If I change fromprivate
toprotected
, eslint complains about naming convention, but I wonder whether it is worth to rename.I'll leave this as a draft for discussion for now, and meanwhile continue to test the extendability on this PR.