webarkit / ARnft

A small javascript library for WebAR with NFT
GNU Lesser General Public License v3.0
219 stars 53 forks source link

Flickering of glTF model #139

Closed brettkromkamp closed 3 years ago

brettkromkamp commented 3 years ago

Hi,

I've been experimenting with displaying glTF models and re-used the glTF example (https://github.com/webarkit/ARnft/blob/master/examples/arNFT_gltf_example.html) to do so. Nonetheless, when rendering the model there is extreme flickering. I have attached two screenshots to try to exemplify the issue. The first screenshot below shows the model using an online glTF model viewer (https://gltf-viewer.donmccurdy.com/).

model-gltf-viewer

The second screenshot is of the actual AR rendering of the model. The model is actively flickering and the overall quality is just very poor.

gltf-render-arntf

So, my question is: am I doing something obviously wrong? As far as I can tell, the model I have used should be suitable for AR purposes (I have used it with AR.js without these kind of issues).

brettkromkamp commented 3 years ago

If necessary, I can provide the (binary) glTF file if that makes trouble-shooting this issue, easier.

brettkromkamp commented 3 years ago

It looks like there is a problem with the normals of the model. I don't believe there is, though.

gltf-render-arntf2

kalwalt commented 3 years ago

Hi @brettkromkamp welcome to WeBARKit and ARnft, and thank you for testing! I would test your model, maybe it can help set logarithmicDepthBuffer to false but you need to add in the ThreejsRenderer constructor a new entry: logarithmicDepthBuffer: false Sorry you can not add custom options to the renderer at the moment...

kalwalt commented 3 years ago

...continuing. You need also to rebuild the project with webpack: npm run build-es6 for a production build or npm run dev-es6 for a development build.

If necessary, I can provide the (binary) glTF file if that makes trouble-shooting this issue, easier.

Yes it would be nice, thank you.

brettkromkamp commented 3 years ago

@kalwalt Hi! Thanks for a reply on a Sunday 👍 I've uploaded the model (zipped because GitHub doesn't allow uploading .glb files directly). I'll take a look at logarithmicDepthBuffer: false as suggested and let you know how it goes.

brave-robot.zip

brettkromkamp commented 3 years ago

@kalwalt Not sure I added logarithmicDepthBuffer: false to the right place (in the ThreejsRenderer.js file):

constructor (configData, canvasDraw, root) {
    this.root = root
    this.renderer = new THREE.WebGLRenderer({
      canvas: canvasDraw,
      alpha: configData.renderer.alpha,
      antialias: configData.renderer.antialias,
      precision: configData.renderer.precision,
      logarithmicDepthBuffer: false
    })
    this.renderer.setPixelRatio(window.devicePixelRatio)
    this.scene = new THREE.Scene()
    this.camera = new THREE.Camera()
}

It made no difference. I'll keep on digging around :)

kalwalt commented 3 years ago

@kalwalt Hi! Thanks for a reply on a Sunday +1 I've uploaded the model (zipped because GitHub doesn't allow uploading .glb > files directly). I'll take a look at logarithmicDepthBuffer: false as suggested and let you know how it goes.

No problem, we are in soft lock down here... :slightly_smiling_face:

@kalwalt Not sure I added logarithmicDepthBuffer: false to the right place (in the ThreejsRenderer.js file):

constructor (configData, canvasDraw, root) {
    this.root = root
    this.renderer = new THREE.WebGLRenderer({
      canvas: canvasDraw,
      alpha: configData.renderer.alpha,
      antialias: configData.renderer.antialias,
      precision: configData.renderer.precision,
      logarithmicDepthBuffer: false
    })
    this.renderer.setPixelRatio(window.devicePixelRatio)
    this.scene = new THREE.Scene()
    this.camera = new THREE.Camera()
}

It made no difference. I'll keep on digging around :)

did you rebuild the project as i explained?

brettkromkamp commented 3 years ago

@kalwalt Yes, I did a development re-build (with npm run dev-es6). Suppose it doesn't make a difference with a production build, but I will try.

I’ll also try with different models.

kalwalt commented 3 years ago

@kalwalt Yes, I did a development re-build (with npm run dev-es6). Suppose it doesn't make a difference with a production build, but I will try.

I’ll also try with different models.

It shouldn't be a difference, the real difference should be in the size of the built file. In theory because ARnft is based on Three.js it should be possible to view the model as in the glTF Viewer. Probably it is needed some extra settings to the renderer.

brettkromkamp commented 3 years ago

@kalwalt Yes, when looking at the source of the glTF viewer, there is a lot of additional effort going in to setting up the renderer, materials and lighting:

https://github.com/donmccurdy/three-gltf-viewer/blob/bf23472b1deddb9904739530bfebf45fef313f67/src/viewer.js#L110 https://github.com/donmccurdy/three-gltf-viewer/blob/bf23472b1deddb9904739530bfebf45fef313f67/src/viewer.js#L381 https://github.com/donmccurdy/three-gltf-viewer/blob/bf23472b1deddb9904739530bfebf45fef313f67/src/viewer.js#L731 https://github.com/donmccurdy/three-gltf-viewer/blob/bf23472b1deddb9904739530bfebf45fef313f67/src/viewer.js#L573

So, I'll play around with this a bit and see how it goes :)

kalwalt commented 3 years ago

I think i need to add an option to inject additional settings to the ThreejsRenderer class, at the moment it is very poor! This is on my radar and to do list!

brettkromkamp commented 3 years ago

That's my thinking, as well. I will try to set some time aside this weekend to work on the issue which will hopefully result in the accompanying PR. I'm also thinking that (hopefully) the glTF Viewer can provide some guidance on how to go about this: https://github.com/donmccurdy/three-gltf-viewer/blob/master/src/viewer.js

kalwalt commented 3 years ago

Thank you @brettkromkamp a PR is welcome!

kalwalt commented 3 years ago

@brettkromkamp For sure i will add other parameters settings in the ThreejsRenderer constructor, but Three.js renderer have a lot of properties and methods: I think that we need to implement a method to retrieve the renderer itself and give us more freedom.

brettkromkamp commented 3 years ago

@kalwalt I haven't got too far with this issue. Perhaps it's better if you take this issue and I'll look around for (or, you suggest) other issues that I can work on. What do you think?

kalwalt commented 3 years ago

@brettkromkamp yes, i will take this issue. Regarding to issues in ARnft, there are different topics,for example I'm working to add support for Aframe ( see PR https://github.com/webarkit/ARnft/pull/110) and Babylon.js (this branch https://github.com/webarkit/ARnft/tree/babylonjs-renderer), we should also improve the Filtering #3; another issue is to try to add new useful methods to ARnft #5. I would also know your opinion about how improve it and if you have other ideas. Feel free to choose your topic. We are open to collaboration and to welcome new members to the webarkit team. :slightly_smiling_face:

kalwalt commented 3 years ago

@brettkromkamp PR #143 is on the way!

brettkromkamp commented 3 years ago

@kalwalt Superficial look... but, that seems like it is going in the right direction :) I'll gladly test.

kalwalt commented 3 years ago

Wait for the next commits in the PR!

brettkromkamp commented 3 years ago

@kalwalt Do you need any testing of the feature-extends-threejsrenderer branch before merging? Or, good to go?

kalwalt commented 3 years ago

Yes, It would be nice some testing, I think i need only to do some corrections/cleaning, you can find my comments in the PR https://github.com/webarkit/ARnft/pull/143

kalwalt commented 3 years ago

@brettkromkamp i merged #143 in the dev branch. If you find some bugs or issues post here, we will fix them.

brettkromkamp commented 3 years ago

@kalwalt Sorry for the delay. Will do!

kalwalt commented 3 years ago

@brettkromkamp i should also add your credits for the model, i will do when i have a bit of time. 🙂

brettkromkamp commented 3 years ago

@brettkromkamp i should also add your credits for the model, i will do when i have a bit of time. 🙂

@kalwalt Thanks. Much appreciated.

kalwalt commented 3 years ago

@brettkromkamp tested the dev branch on Mobile device. The model looks much, much better than the initial case. I think i will merge it. Model appears shifted (not centered on the pinball image), don't know if it is caused by the new changes.

kalwalt commented 3 years ago

See this screenshot: Screenshot_20201210-220149

brettkromkamp commented 3 years ago

@kalwalt Model looks much better (in the above photo). I will test against the development branch, as soon as possible. The model could be offset... perhaps the origin has changed? I'll check.

kalwalt commented 3 years ago

@brettkromkamp thank you! No, i tested now also the simple gltf example with the duck model and it is affected by the same issue. I think it is not caused by the new code. I will open a new issue to track this last problem.

brettkromkamp commented 3 years ago

@kalwalt Ok. Anyway, testing of rendering of the Brave Robot model is looking much better.

arnft-brave-robot-test.

We can probably close this issue then, or? :)

kalwalt commented 3 years ago

Well, i think it is a big difference! .. and it looks better on your picture! I will close this isssue when i will merge it in master. Thank you again! 😄

kalwalt commented 3 years ago

Now the new features are in master branch!

jalamprea commented 3 years ago

I know this issue is closed, but did you know the issue is still presenting? the flickering on gltf models is appearing even on the basic sample with the Duck model. On my end I had to replace the pure Three Camera for a Perspective camera, adjust the fov to 40, and use the own camera.updateProjectionMatrix() method instead of set the matrix from the artoolkit. Now it works so much better.

kalwalt commented 3 years ago

@jalamprea You should check the brave robot example but yes probably the basic gltf example need also these changes, I want to point out again that is not an issue with the tracking algorithm, but only with the rendering engine, in this case Three.js; because we plan to develop a library that is not rely to a specific rendering engine we are removing the ThreejsRenderer class and developing an external package for this see PR #180