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

Bump Three.js into r128 #4321

Merged
merged 46 commits into from
Jul 27, 2021
Merged

Bump Three.js into r128 #4321

merged 46 commits into from
Jul 27, 2021

Conversation

takahirox
Copy link
Contributor

@takahirox takahirox commented Jun 9, 2021

Fixes: #4108

Requires: Hubs-Foundation/aframe#30
Requires: InfiniteLee/three-to-ammo#25
Requires: InfiniteLee/buffered-interpolation#7
Requires: Hubs-Foundation/three-bmfont-text#1
Requires: InfiniteLee/three-ammo#22
Requires: Hubs-Foundation/lib-hubs#30

Description

This PR upgrades Three.js into r128. Currently the latest revision is r129 but the revision was r128 when I started to work on this. We can upgrade to r129 later. It should be much easier than upgrading from r111 to r128.

Please refer to #4108 for the purpose, benefits, and changes.

Notes

  • r128 Three.js dropped deprecated WebVR API support. I import webxr-polyfill from hubs.js for platforms supporting only WebVR e.g. Desktop Firefox + Wired VR device. I don't have such devices. If you have, would you please test to check if it works?

  • I didn't apply this change so far because I don't see any errors so far. I guess it's from batching manager system or Three.js revision older than r128? Let me know if you encounter any shader material problems.

Hubs-Foundation/three.js@d83c538 Fix defines in RawShaderMaterial

  • I dropped the change for Three.js TextureLoader so far. Instead I refactored HubsTextureLoader to follow the Three.js common Loader API and let Hubs source code explicitly use HubsTextureLoader rather than hacking Three.js TextureLoader for ImageBitmap. Only the place which uses Three.js TextureLoader I noticed is A-Frame material system. Do you folks know what the system is for? And should we use ImageBitmap for A-Frame material system by hacking TextureLoader although we need to sacrifice the maintenance cost?

  • I disable to use ImageBitmap for Firefox. Background is WebGLRenderer: Avoid default color space conversion. mrdoob/three.js#21336. createImageBitmap() with option fails on Firefox due to the bug. r128 Three.js ImageBitmapLoader passes {colorSpaceConversion: 'none'} option. Without the option, the rendering result can be wrong if image file has ICC profiles. We may have an option to hack ImageBitmapLoader for Firefox not to pass the option. It's the tradeoff between rendering result vs maintenance cost. If we want to enable ImageBitmap for Firefox, I want to carefully change it for example researching how many ICC profiles use cases (I guess it may be rare tho) and want to do in another PR.

  • It seems webpack tree shaking removes the modules under the r128 three/examples/js if I use import. Somehow it's resolve by using require instead. There might be better solutions but I use require in the PR so far.

  • We disabled Batching manager system but it imports dependencies which break the build with Three.js r128. Then I replaced Batching manager system with a stub so far. We can revert it when we will enable it again.

  • It seems this change is for Oculus browser specific performance problem. There is a chance that the problem has been already resolved. So I dropped this change so far. Let me know if you still see performance problem. We may be able to upload at the start with renderer.initTexture() without hacking Three.js.

Hubs-Foundation/three.js#32 Add option to upload videos at the start render

  • Regarding the flipY + PlaceBufferGeometry, I made a utility function and let Hubs use it instead of hacking Three.js.

Hubs-Foundation/three.js#21 Prefer ImageBitmapLoader and dispose of textures after GPU upload #21

Test

Three.js is one of center cores in Hubs client and it's relevant to almost all the features in Hubs. Would you please help the test to check if all the functionalities are not broken? Please locally merge this PR for the test.

For Hubs team internal: I set up a testing instance. You can use it for the test. See the discord for the URL.

If you find any new bugs, please report in this thread.

Once I confirm that basic functionalities doesn't seem to be broken, I will work with the QA team for more systematic and better coverage testing.

I will merge this PR after we finish the tests with the QA team.

TODOs

  • Resolve the conflicts
  • Merge PRs in dependency repos
  • Update package.json and package-lock.json

takahirox added 27 commits June 2, 2021 12:02
…bGLRenderer standard API .compile() and .initTexture()
…ink to the official one when it will be updated.
… to the MozillaReality fork when it will be updated.
@takahirox takahirox force-pushed the LatestThree branch 2 times, most recently from 3a5602f to 4add8de Compare June 9, 2021 20:02
@takahirox
Copy link
Contributor Author

A bug on Android Chrome.

physics-utils.js console error when entering a lobby. I don't see this error on desktop.

Fixed this issue. I needed to update physics-utils.js to follow the latest three-mesh-bvh API.

It seems webpack tree shaking removes the modules under the r128 three/examples/js if I use import. Somehow it's resolve by using require instead. There might be better solutions but I use require in the PR so far.

This happens only with webpack production mode. We may add npm run dev-production command to run dev with production mode. If this command existed I could notice the problem before deploying to the testing instance.

Looks like its using PlaneBufferGeometry here https://github.com/MozillaReality/lib-hubs/blob/master/packages/three-particle-emitter/src/index.ts#L125 ... I can make a PR to fix that.

Good catch. I didn't know well about lib-hubs. Should we move createPlaneBufferGeometry util function there?

@takahirox
Copy link
Contributor Author

takahirox commented Jun 12, 2021

@netpro2k I confirmed that your recent two commits resolved all the useProgram issues I posted above in low material quality mode (= Android Chrome). Nice work!

So, only the problems we noticed and are not resolved yet are upside down small emojis and loading cube transparent issues, aren't they.

And the platforms we didn't test well yet may be

  • Android Firefox
  • VR Headset + WebXR
  • VR Headset + WebVR (e.g. Desktop Firefox + wired VR headset)
  • iOS
  • Linux

I'm happy if you folks test the basic functionalities on them if you have some of them.

@netpro2k
Copy link
Contributor

I have done all my testing in Linux so far, so that's covered.

So, only the problems we noticed and are not resolved yet are upside down small emojis and loading cube transparent issues, aren't they.

Jim provided a new GLB for the loading cube, I will replace it. And yep the emoji issue is the flipy problem, so an easy fix.

@takahirox takahirox mentioned this pull request Jun 17, 2021
@takahirox
Copy link
Contributor Author

The QA team seems to be busy for two-three weeks.
So I would like to finish the basic functionality check by they will have time.

I'm testing Oculus Quest + Oculus browser. I succeeded in entering immersive mode. I found a bug that the hands are not displayed but others looks fine. I'm investing the missing hands problem.

@takahirox takahirox self-assigned this Jun 21, 2021
@takahirox
Copy link
Contributor Author

I found a bug that the hands are not displayed but others looks fine. I'm investing the missing hands problem.

Sorry I just used an avatar which doesn't hands. I switched it to another one having hands, the hands are displayed.

@takahirox
Copy link
Contributor Author

I'm testing Oculus Quest + Firefox Reality and encountered a problem that I can't enter an immersive mode.

The error log is

image

@takahirox
Copy link
Contributor Author

I have fixed the setDevice() error.

And now I have a warning and an error on Firefox Reality.

image

I would like to suggest to ignore the both so far because there seems no function problem.

The details are following.

1. "XRWebGLLayer antialiasing is not supported. Antialiasing will be diabling" warning

XRWebGLLayer.antialias is unsupported by Firefox.

Firefox reality seems to support WebVR and WebXR APIs by default. The current Hubs uses WebVR API if the both WebXR and WebXR APIs are available. Firefox Reality WebVR seems to allow antialiasing. I speculate this is the reason why antialiasing works on Firefox Reality in the current hubs.mozilla.com.

I would like to suggest to just ignore this warning and accept non-antialiasing on Firefox Reality. Because I don't want us to the keep WebVR API specific codes only for Firefox Reality. (WebVR API is already deprecated.)

2. "Uncaught (in promise) TypeError: Failed to find attached VR displays" error.

This error seems to be caused by the simultaneous WebXR and WebVR request according to the Gecko code.

With $ find ./src -print0 | xargs -0 grep "navigator\.getVRDisplays" command actually I have found some codes like

if (navigaror.getVRDisplays) {
  navigator.getVRDisplays()...
}

According to the Gecko code, it seems we can ignore that error without any problems. I confirmed that I can enter the immersive mode.

We are going to drop WebVR related code (unless there is any problems) so ideally we should remove such codes at some point and probably the error will be disabled. (I don't want to include such clean up in this PR because I don't want to make the PR bigger.)

@takahirox
Copy link
Contributor Author

I tested the very basic function (enter lobby/room/immersive mode, move, VR controls, and etc.) on Oculus Quest + Oculus Browser and Firefox Reality, Android Chrome/Firefox, and iOS safari. @netpro2k tested on Linux. So remaining platform we should test is

  • VR Headset + WebVR (e.g. Desktop Firefox + wired VR headset)

And remaining bug we should fix is

  • Upside-down small emojis

I would like to finish them and test more basic functions (e.g. placing an object, switching a scene, etc) in the two-three weeks. And then I would like to ask the QA team for the test as I mentioned above.

@takahirox
Copy link
Contributor Author

Fixed the upside-down small emojis problem.

@takahirox takahirox merged commit e9b7a64 into master Jul 27, 2021
@takahirox takahirox deleted the LatestThree branch July 27, 2021 22:43
@takahirox
Copy link
Contributor Author

takahirox commented Jul 28, 2021

Three.js for Hubs has been upgraded to r128, thanks everyone who helped me!

Now, the latest Three.js revision is r130 (and I guess r131 will be released soon).

https://github.com/mrdoob/three.js/releases

I'm wondering how often should we keep track of Three.js releases? Some options in my mind

  1. Always use the latest revision.
  2. Upgrade Three.js for every some Three.js releases (for example every three releases).
  3. Track the change log of Three.js and upgrade Three.js only when the benefit of upgrading looks big to us.
  4. The mix of 2 and 3

The pros and cons of more frequent upgrade are

  • Pros:
    • Newer Three.js should have better optimization, stability, and new features. We can get the benefit of them more quickly.
    • One upgrading cost would be relatively smaller. Even if Three.js has breaking changes, it would be easier to follow and test than less frequently upgrading it.
  • Cons:
    • Three.js is released every month. Three.js is one of core libraries for us and we should test well, ideally we should entirely test Hubs Client, for each upgrading. Testing well for example every month might be a bit costly.

and the pros and cons of less frequent upgrade are opposite to them.

Any thoughts? I haven't deeply thought yet but for me basically upgrading every one or two three months + sometimes urgent upgrade for bug fix may be good?

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.

Replace our Three.js fork to the official one
2 participants