-
Notifications
You must be signed in to change notification settings - Fork 12
Add xr-active-switch.ts #72
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
base: main
Are you sure you want to change the base?
Conversation
Squareys
left a comment
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 will be such a nice component!
d54b8ab to
ffddab3
Compare
xr-active-switch.ts
Outdated
| * that depend on XR session states. | ||
| * | ||
| */ | ||
|
|
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.
Superfluous newline here
xr-active-switch.ts
Outdated
| const mode = this.scope; | ||
|
|
||
| // Handle current object | ||
| if (mode === 0 || mode === 2) { |
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.
Maybe either add comments like 0 /* this object */, or an enum, such that this reads mode == Scope.ThisObject
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 enum can also be exported and this.scope can be typed to it (I think), such that people can easily add them at runtime and just this.object.addComponent('xr-active-switch', {scope: Scope.ThisObject, ifInXR: Action.Activate});
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.
Ah, there it may make sense to make the defaults Action.Keep, since the most sensible default is that leaving out an explicit configuration just does nothing.
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.
And maybe we call it Action.None? That's a bit clearer than Keep, also for the enum value 'none'
d4564c6 to
0db127f
Compare
0db127f to
41cddfb
Compare
41cddfb to
eb956da
Compare
| private processChildren(obj: Object3D) { | ||
| for (const child of obj.children) { | ||
| child | ||
| .getComponents() |
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.
getComponents(XrActiveSwitch.TypeName) already achieves the filtering
| } | ||
|
|
||
| private applyAction(action: Action) { | ||
| this.collectComponents(); |
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.
Since you only use them here, no need to store on the class:
processChildren(obj: Object3D, out: Component[]) {
// Push into `out`
}
collectComponents() {
const components = [];
processChildren(..., components);
return components;
}components is only temporary, doesn't need to live in the class
No description provided.