-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Post-Process example #5648
base: master
Are you sure you want to change the base?
Post-Process example #5648
Conversation
Where does all those 1087 lines of code comes from? |
Yes, thanks for pointing that, I completely missed the new feature of A-Frame to use importmap. Sorry for that, I've reduced the bloom.js to the bare minimum to work. |
That new importmap example exist since 2 days ago ;) I'm glad I did it so we can have a simpler example here without copying all the code. |
Didn't we agree from supermedium/three.js#20 to use |
Whatever yields 90fps with the simpler code and least amount of dependencies |
pmndrs does not work in VR at the moment. I have opened tickets there, but at the moment no one is working on it. |
Typo Co-authored-by: Noeri Huisman <[email protected]>
deleted useless tick function Co-authored-by: Noeri Huisman <[email protected]>
I merged THREE changes and updated A-Frame so should work on top of master. Can you put the examples under Thanks so much for all the effort |
examples/post-process/bloom.js
Outdated
this.scene = this.el.object3D; | ||
this.renderer = this.el.renderer; | ||
this.camera = this.el.camera; | ||
this.composer = new EffectComposer(this.renderer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of recreating EffectComposer
, RenderPass
and UnrealBloomPass
each update, they can be created once in init
and only updated here.
examples/post-process/bloom.js
Outdated
}, | ||
bind: function () { | ||
const render = this.renderer.render; | ||
const system = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: this isn't a system, probably better to name it self
.
examples/post-process/bloom.js
Outdated
* Unreal Bloom Effect | ||
* Implementation for A-Frame | ||
* Code modified from Akbartus's UnrealBloomPass.js | ||
* https://github.com/akbartus/A-Frame-Component-Postprocessing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the source is MIT licensed, the original copyright and license notice should be included.
examples/post-process/index.html
Outdated
"imports": { | ||
"aframe": "../../dist/aframe-master.module.min.js", | ||
"three": "https://cdn.jsdelivr.net/npm/[email protected]/build/three.module.js", | ||
"three/addons/": "https://cdn.jsdelivr.net/npm/[email protected]/examples/jsm/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be
"three": "../../../super-three-package/build/three.module.js",
"three/addons/": "../../../super-three-package/examples/jsm/",
like https://github.com/aframevr/aframe/blob/master/examples/boilerplate/importmap/index.html
so it uses the three version from node_modules.
examples/post-process/index.html
Outdated
shadow="type: pcfsoft; autoUpdate: true" | ||
background="color:black;" | ||
renderer="anisotropy:4; stencil:true; alpha:false; colorManagement:true; exposure:1.0;" | ||
bloomm="threshold: 1.0; strength: 0.6; radius: 1; exposure: 1.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the example still work? there is two m here.
Interesting. I think we are seeing different results for some reason. This is what I see with your command, Quest 3 and https://incluverse.eu/examples/bloomtest5.html Result matches what I see in the OVR Metrics Tool HUD. I'm in v72 if that makes any difference.
|
So instead you want people to test that all is well on everything, including emulator, but breaks on your devices? This isn't just post-processing, but anything offscreen rendering, and renderer state is permanently broken! Who are you to even decide this for everyone? Should we do feature detection to disable the most requested feature in three.js for iPhone too? I'm sure Apple themselves would be happy to see their own pages no longer work. My entire career in real-time graphics has been on handheld mobile/TBDR, and even without post-processing, I've used these (currently broken) codepaths to simply compromise and perform basic upsampling or blitting, which people should be doing in XR also. I couldn't disagree more with this notion, as you are actively gatekeeping in the process and preventing the rest of us from further improving this area. If you really want to shut down or remove anything, it should be three's (unmaintained) upstream implementation, not the whole ecosystem with it. That is why I find this in bad faith since the strategy here is self-sabotaging in my view, and I'm not willing to believe either of you didn't observe nor foresee this, nor acknowledge that this could be a simple combination of poor parameters.
There are no other PRs on our side; they must be made in pmndrs/postprocessing, and they can't be done without upstream improvements. This is what I mean by the ecosystem being in stasis, and it doesn't stop there but anything that uses offscreen rendering (shadows, impostors, LUTs -- although these should probably be parametric functions aside from color grading). Most of Drei is affected, for instance, and it would take many months to fix all the components. |
I'm happy to take those improvements if there's a reasonable path to maintain them until upstream concerns are resolved |
I don't think you understood what I meant, but those libraries I maintain in Poimandres, and upstream means three.js or mrdoob/three.js#26160, which is among the simplest PRs I've made to three.js and by far the most valuable at $10,000 since I thought that would motivate people to repeat my own work and see past this nonsense. It's not the most complicated to research or repeat, and the changes are still exhaustive today. Again, I'm happy to personally issue devices or maintain this myself, and I've said elsewhere, but I will award proof of existing apps regressing with $5,000 as I mean when I say "strict improvement". I put an additional $500 here since I feel the same responsibility with aframe, and I only see Rik shutting this down no matter what I do myself. I don't believe him for a second. All of my time has been wasted, and I preach to deaf ears. Not letting that continue to happen. |
I think understood and probably talking past each other. What I meant is that I would be love to recreate whatever optimizations / approach exist or have in mind for Poimandres in A-Frame. Poimandres is great but don't want to take in new dependencies just yet. If there's any more pending three work happy to take those changes in our fork https://github.com/supermedium/three.js We already have mrdoob/three.js#26160 Is that all? If it is. Does Poimandres have a bloom approach that is better than in this PR? |
Thanks for upping the bounty. I think will get something out for people to play with in A-Frame 1.7.0 |
I give up and retract all bounties and pending work. Meta won. |
Well, this PR is full of life.. good sign. I invite everyone to keep this discussion useful and not toxic, I personally have thousands of other things to do, but I choose to spend some of my time to help the WebXR and XR community to have better tools to work with, but I have limited skills and I cannot contribute as I would, but you all are the very experts in this field and you should cooperate to improve things.
Now, in order to proceed I think we should:
Thanks |
I have the same metrics, 90fps on Quest 3 and 30fps on Quest 2. |
Another 2 cents: |
@vincentfretin Thanks so much for confirming the numbers @enzofrancescaHM Thanks for the patience and all the hard work. This is pretty close. Can we incorporate your car example to this PR (2-3 lights up to you)? We can improve after merge. I’m super happy to have something we can iterate over. |
OK, done. Css is local, model is in te model folder of A-Frame Examples |
Thanks. Can you submit the model into the assets repo? https://github.com/aframevr/assets/tree/master/examples create a directory for the example under ‘examples’ |
done. |
The car is available now via CDN: https://cdn.aframe.io/examples/post-processing/fancy-car.glb |
For model credit and other instructions we use a "standard" info panel: https://aframe.io/aframe/examples/showcase/comicbook/ You can just import the info-message component and use it on the scene |
examples/index.html
Outdated
@@ -158,6 +158,7 @@ <h2>Examples</h2> | |||
<li><a href="mixed-reality/anchor/">Anchor (Mixed Reality)</a></li> | |||
<li><a href="mixed-reality/real-world-meshing/">Real World Meshing (Mixed Reality)</a></li> | |||
<li><a href="boilerplate/importmap/">Importmap (import teapot geometry from three/addons)</a></li> | |||
<li><a href="showcase/post-processing/">Post-Processing (bloom effect)</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add (bloom effect)
for simplicity
Here's a proof-of-concept of a Bloom effect mostly hitting 90fps on a Quest 2: https://thrilling-alkaline-headline.glitch.me/ This is not meant to further complicate or delay this PR. Just curious if it was doable and with what trade-offs, as that might help determine if we could/should pursue it. Even with the current performance implications this PR can be merged IMHO. In fact, the As @dmarcos said, once merged we can iterate over it. If post-processing ever becomes a core feature, then we should be a lot more careful. |
Any improvements you can suggest on this PR for Quest2? Two paths for Quest 2 and 3 is acceptable. Thanks! |
Wow, amazing work @mrxz ! Yes, as you were saying the effect seems a little bit cheaper than the one on this PR, but it is super acceptable and nice. Maybe two paths with different levels of quality / performance are the right way. And, I agree that we should merge and than iterate over it. |
Haven't tried in VR. Is the "more expensive" one noticeably better visually? If yes two paths is fine. If not maybe we can just have the cheaper one. |
The But it's actually possible to get closer while still (though barely) maintaining 90fps on Quest 2. See: https://thrilling-alkaline-headline.glitch.me/index2.html
But it becomes questionable how useful a post processing effect is that eats up practically all your rendering budget.
The biggest gains are in reducing the render targets used while rendering, but this is mostly caused by the internal implementation of the One thing that can be done is optimizing the car asset using gltf-transform, as it does cause more draw calls than needed. But that isn't directly related to the bloom effect, of course. |
I think your new iteration looks very good! Yes, it is very veery similar to the PR's one. |
How much of the rendering budget is used in Quest3? How much is reasonable? Any target we should aim at? |
Description:
Minimal example to show the possibility of implementing Post-Processing in A-Frame.
NOTE: for Post-Processing to work also in VR mode, supermedium/three.js#20 must be implemented in supermedium three.
Changes proposed:
-index.html running a simple scene with simple geometries with and without emission
-bloom.js, a minimal implementation of Bloom effect