search

NeoSpark314 / godot_oculus_quest_toolkit

An easy to use VR toolkit for Oculus Quest development using the Godot game engine
MIT License
367 stars 38 forks source link

Improvements on Beep Saber #35

Closed Joanguitar closed 4 years ago

Joanguitar commented 4 years ago

Changes:

-Compatible with OpenVR (and thus SteamVR). -Saber cuts now depend on the movement of saber's tip instead of its base. -Blocks now have inertia when cut -Menu for playlists, songs and difficulty -Not found songs automatically get downloaded from BeatSaver (songs are identified by their id) but they don't get uncompressed (lack of zip support in current Godot version) -Songs now load from .egg files (BeatSaver's format)

I hope you find this changes usefull, I really like this game

NeoSpark314 commented 4 years ago

Wow; that looks like a big change :-); I will give it a closer look later this week and will also test it on the quest. I might also post some questions inside the code review. But from what you wrote this sounds like awesome additions to the game.

NeoSpark314 commented 4 years ago

could you also maybe squash all the commits to a single one; this would make reviewing a bit easier as you have several things later removed in another commit and I find it hard to keep track of what was just temprorary and what is still in the final version. Overall I think it will require some changes to work on the oculus quest; my suggestion would be that once you cleaned up the PR (by removing the song binaries and also the openvr binary and all other files that are not needed) we merge it into a branch first and go from there to make it work in the context of the quest.

Joanguitar commented 4 years ago

I'm a bit undecided if it is a good idea to add the openvr addon by default to the repository or if it is better to document that users need to download it from asset library when they want to use it with openvr. The same goes for the oculus desktop plugin (and probably in the future openXR).

The OpenVR plugin is 623KB, literally half the size of the ovrmobile one, if we remove it only Quest users will be able to use it without recompiling it from Godot, which is a huge turnoff for people that don't know how to use it. Most VR users don't even use Oculus source.

I think we should not add additional binary files to the main repository to keep the download size as small as possible; also in this case this is non-free music that we definitely should not distribute. We need to be careful with the licensing; could you remove the added song from the commit?

You're completely right, I didn't know what music to select, do you have any recommendations? Is the music from TheFatRat not licensed?

I'll squeeze the commits into a single one to keep better track of changes.

NeoSpark314 commented 4 years ago

Regarding the OpenVR Plugin: you are right that it is small; but it is also available from the asset library so a user would not need to compile it; and I have heard that it will conflict with the oculus desktop plugin if someone would like to use that one (still need to test this though); But having things work "out of the box" is also nice... so as I said; I'm a bit undecided :-) but we should at least setup export filter so that it does not get deployed to oculus quest.

The song from TheFatRat was marked as no copyright on the composers youtube channel. I think a single song is enough as an "included" sample song; especially if there is now the option to download and add your own songs now.

Joanguitar commented 4 years ago

I used songs from TheFatRat, as the author specifies in its YouTube channel they are marked as no copyright and squashed all commits into a single one.

NeoSpark314 commented 4 years ago

Thanks; I will merge it into a separate branch first and will test on the weekend if everything works on the Oculus Quest before merging it into master.

Regarding the songs: since this is first a toolkit to be used in general VR projects I think including more than a single song mightl make the download size unnecessary large for most people. What do you think; is the original song I included enough?

NeoSpark314 commented 4 years ago

but we could put the song pack as a download in the releases I think

NeoSpark314 commented 3 years ago

Hi @Joanguitar; just wanted to let you know that I decided to put BeepSaber as a fork now in it's own repository here: https://github.com/NeoSpark314/BeepSaber so far it is based on the last changes you made; having it stand alone should make it easier to add improvements. Only your download code is currently disabled as this does not work that way on the Oculus Quest