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

Return boolean from fill methods #46

Merged
merged 2 commits into from Sep 15, 2020
Merged

Return boolean from fill methods #46

merged 2 commits into from Sep 15, 2020

Conversation

Manishearth
Copy link
Contributor

@Manishearth Manishearth commented Sep 11, 2020

This lets us do things like

if (fillPoses(spaces, poses)) {
  renderHand(poses)
}

Another part of #37, was missed in #44


Preview | Diff

@@ -301,9 +301,9 @@ XRFrame {#xrframe-interface}
<pre class="idl">
partial interface XRFrame {
XRJointPose? getJointPose(XRJointSpace joint, XRSpace baseSpace);
void fillJointRadii(sequence&lt;XRJointSpace&gt; jointSpaces, Float32Array radii);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unsure if this one needs to be a bool too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well, for consistency. It's not going to be as frequently used as the return value from fillPoses, but it'll definitely bite someone if the return value is different between them.

@Manishearth
Copy link
Contributor Author

Should the boolean be false when there are any null poses or false when there are all null poses?

@Manishearth
Copy link
Contributor Author

/agenda to discuss if this is necessary

@probot-label probot-label bot added the agenda label Sep 11, 2020
@toji
Copy link
Member

toji commented Sep 11, 2020

enum XRTruthiness {
  'true',
  'false',
  'space_not_found'
};

😁

More seriously, though, I have an inclination to say that it should return false if any poses failed to resolve, since given the code snippet you posted seems extremely reasonable/likely but it could fall apart fast if the result has NaN's sprinkled throughout. Someone who is trying to handle NaNs correctly is going to be forced to loop through and check every value anyway, so there's little reason for them to check this value (aside from possibly a "you can take the fast path this frame" signal?)

@Manishearth
Copy link
Contributor Author

That sounds good to me!

@Manishearth
Copy link
Contributor Author

That makes this great for hands, but more annoying for anchors, where a false value would still mean you need to iterate through it, but maybe filtering it.

Copy link
Member

@cabanier cabanier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with returning false if any pose or radius fails to resolve.

@Manishearth
Copy link
Contributor Author

Manishearth commented Sep 11, 2020

Flipped the return value to be "if all are valid" instead of "if any are valid"

@Manishearth
Copy link
Contributor Author

Okay, there seems to be consensus on merging this, and the radius thing can be improved after we fix #46 .

@Manishearth Manishearth merged commit 58a23b1 into master Sep 15, 2020
@Manishearth Manishearth deleted the fill-bool branch September 15, 2020 19:25
@Yonet Yonet removed the agenda label Sep 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants