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

Refactor / p5xr consolidation / Fix AR mode #215

Merged
merged 11 commits into from
Jul 29, 2024

Conversation

TiborUdvari
Copy link
Collaborator

As discussed in #207, this PR consolidates common code between p5xr and p5ar into p5xr in preparation for a more feature based approach. Initially I was looking to fix AR mode for the use with my Quest 3, but doing the refactor at the same time made the most sense.

Let me know if you would like me to squash the commits into a single one, or if you prefer "atomic commits".

Here are the changes in reverse chronological order:

  • Hello cube AR example
  • Fix tests for launching from constructor
  • Consolidated common startsketch and request session into p5xr
    Removed p5.instance._decrementPreload as it was launching p5 in 2D mode for
    some reason
  • Moved common requestSession code into p5xr from p5ar and p5vr
  • Favour explicited WebXR modes
  • Moved isImmersive to XR level, AR and VR can both be immersive
  • Removed superfluous init functions, create buttons in constructors
    Removed button argument because it was always created and overloaded afer
    construction
  • Removed deprecated navigator.getUserMedia()
    https://developer.mozilla.org/en-US/docs/Web/API/Navigator/getUserMedia

@TiborUdvari TiborUdvari mentioned this pull request Apr 8, 2024
@stalgiag
Copy link
Owner

This looks exciting! I just read through the code and everything makes sense. Later this month I can get my hands on a headset for testing but let me know if you would prefer a more expedited timeline. I would also be open to the idea of promoting your permissions and allowing you to merge sooner and I can follow up with bug reports if something is amiss once I am able to actually test.

@TiborUdvari
Copy link
Collaborator Author

Thanks! There is this Immersive Web Emulator which is nice for testing.

I think I prefer to merge once you could check things. I setup npm publishing from my fork as well, I think it's useful for testing out features where it's unclear what the p5 like api should be, like in #216

Since there don't seem to be any other active contributors at the moment I think the risk of conflicts is pretty minimal.

So better keep it as is for now IMO.

@TiborUdvari TiborUdvari mentioned this pull request Apr 18, 2024
Copy link
Owner

@stalgiag stalgiag left a comment

Choose a reason for hiding this comment

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

Nice. This is a pretty great improvement for such a small set of changes. I am okay with merging whenever you are ready.

this.xrSession = null;
this.xrRefSpace = null;
this.xrViewerSpace = null;
this.xrHitTestSource = null;
this.frame = null;
this.gl = null;
this.curClearColor = color(255, 255, 255);
Copy link
Owner

Choose a reason for hiding this comment

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

curious about this one!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, silly mistake

this.viewer = new p5xrViewer();

this.requiredFeatures = requiredFeatures;
this.optionalFeatures = optionalFeatures;
Copy link
Owner

Choose a reason for hiding this comment

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

This interface might be interesting to offer flexibility from the user's perspective in the future.

// });
// this.addInlineViewListeners(this.canvas);
// }
// });
Copy link
Owner

Choose a reason for hiding this comment

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

I am a fan of either commenting to explain a commented out chunk or just deleting the deprecated code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm putting this back, this is necessary for inline mode to work

@TiborUdvari TiborUdvari merged commit 26f918b into stalgiag:main Jul 29, 2024
1 check passed
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.

2 participants