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

Draft: [1.20.4] Many API Additions #255

Open
wants to merge 25 commits into
base: Multiloader-1.20.4
Choose a base branch
from

Conversation

hammy275
Copy link
Contributor

@hammy275 hammy275 commented Feb 24, 2024

A 1.20.4 port of #184 . This also adds a couple of new changes:

  • Moved isVanillaRenderPass() to the bottom of the file, since its still marked @Beta. Perhaps we want to make a VivecraftRenderingAPI or something, just for organization?
  • Added isFirstRenderPass() and getCurrentRenderPass(), as requested.

All features can be tested with this test mod. Note that the test mod itself has only been tested on Fabric, but should work in Forge.

@hammy275
Copy link
Contributor Author

hammy275 commented Aug 30, 2024

Rebased onto Multiloader-1.20.4. Haven't tested it, but there were only a handful of merge conflicts with very simple fixes for all of them. Should be good to test with the test mod linked above.

Unrelated, but if it makes life easier, I'm happy to rebase this after #284 is merged.

@hammy275 hammy275 force-pushed the vivecraft-api-1.20.4 branch from e8198a4 to ab8b5d1 Compare December 1, 2024 20:51
@hammy275 hammy275 changed the title [1.20.4] Many API Additions Draft: [1.20.4] Many API Additions Dec 1, 2024
@hammy275 hammy275 marked this pull request as draft December 1, 2024 20:52
@hammy275
Copy link
Contributor Author

hammy275 commented Dec 1, 2024

This PR is now caught up with the big refactor, but is untested. That said, I've marked it as draft, as there's several changes I'd like to make, and because the catching up to the refactor is entirely untested beyond ensuring it builds. These changes include:

  • Rebase and test that everything still works after all the rebasing. The demo mod I made should help with that significantly.
  • FBT support.
  • The renaming of reversed hands to left-handedness
  • Refactoring to not have two versions of VRPose and VRPoseImpl. Ideally, just the interface and the class should exist, without needing to do internal conversions for the API. I'm actually deciding not to pursue this, mainly because it means we have multiple spread-out implementations of things like error-handling in the API (such as invalid parameters being passed). As such, I'm leaving this all in one area and doing conversions for API-sake.
  • Rename ClientAPIImpl and APIImpl to VivecraftClientAPIImpl and VivecraftAPIImpl.
  • Move rendering stuff out of the client API and into a "rendering API" area.
  • Move haptic pulse triggering into VRPlayer, since it makes sense that once you have a VRPlayer, you should be able to trigger a haptic pulse on it. Given the rename of VRPlayer which is now VRPose, this no longer makes sense to do.
  • Rename VRData and VRPose to VRPose and VRBodyPart respectively. Maybe drop "VR" from the front of these names? Deciding not to so it's clear from other objects that may not be VR-related, such as entity Poses in Mojmap.
  • Capitalization of "VR" vs. "Vr" consistency in the API.
  • Refactor VRBodyPartHistory to just capture the entire VRPose instead.
  • Remove methods from VivecraftClientAPI that duplicate functionality from getting VRPoses. makes sense to both have a convenience method and one in VRPose for common use.
  • Consistent naming between VRBodyPart and the VRPose getter methods.
  • Rename API classes to be less wordy (Vivecraft -> VR, such as VivecraftClientAPI -> VRClientAPI).
  • Likely need to adjust trackers to not accept the dataholder and Minecraft forcibly, since the API doesn't intend to provide the latter and can't provide the former since the dataholder is outside the API. Not needed, this application is only done internally.
  • Expose methods from RenderHelper. See the discussion about it
  • Better naming around methods above (getControllerRenderPos and setupRenderingAtController)
  • Update demo mod to include the above changes and test test test.

@hammy275 hammy275 force-pushed the vivecraft-api-1.20.4 branch 2 times, most recently from 426b2fe to b8c7771 Compare December 4, 2024 17:53
@hammy275
Copy link
Contributor Author

hammy275 commented Dec 4, 2024

Squashed everything into one commit for now to make rebasing less annoying and now rebased on top of the JOML changes. Still need to test and make the changes above, but that should happen soon.

@hammy275 hammy275 force-pushed the vivecraft-api-1.20.4 branch 2 times, most recently from fc4e468 to 83c153b Compare December 27, 2024 19:55
@hammy275 hammy275 force-pushed the vivecraft-api-1.20.4 branch 6 times, most recently from dea1552 to 241a39e Compare January 13, 2025 01:38
@hammy275
Copy link
Contributor Author

For those following this, the migration to VRPoseHistory introduced the very stupid deviceNum. This is just a placeholder before I add FBT support and this gets replaced with a proper enum.

@hammy275 hammy275 force-pushed the vivecraft-api-1.20.4 branch 3 times, most recently from e416d67 to 838c872 Compare January 15, 2025 20:50
@hammy275
Copy link
Contributor Author

Marking this as ready for review. The test mod is updated at https://github.com/hammy275/mc-vr-playground/releases/tag/v2.1.0, which should allow testing the vast majority of the API's functionality. I'm looking for all forms of feedback imaginable, whether it's bugs, feedback on API-design (big or small), or anything else.

There is definitely more I want to see added to the API, such as what's mentioned in #335, but this PR is already really big in-scope, so I'd rather get this merged so future, smaller API additions can be made separately than continue to delay this PR.

@hammy275 hammy275 marked this pull request as ready for review January 16, 2025 17:49
Copy link
Member

@fayer3 fayer3 left a comment

Choose a reason for hiding this comment

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

looked though, and added some notes

common/src/main/java/org/vivecraft/api/client/Tracker.java Outdated Show resolved Hide resolved
common/src/main/java/org/vivecraft/api/client/Tracker.java Outdated Show resolved Hide resolved
common/src/main/java/org/vivecraft/api/client/Tracker.java Outdated Show resolved Hide resolved
@hammy275 hammy275 force-pushed the vivecraft-api-1.20.4 branch from 6bb5499 to 88e8924 Compare January 17, 2025 17:27
@hammy275
Copy link
Contributor Author

hammy275 commented Jan 17, 2025

The review as it was should be addressed at this point. Going to update MC VR Playground so some testing can be done on the PR since it hasn't been tested since the review was addressed and it currently doesn't cover any of VRPoseHistory outside of getting the literal history.

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