-
Notifications
You must be signed in to change notification settings - Fork 434
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
Use Phong shader for rendering scenes #152
Conversation
using namespace Magnum::Math::Literals; | ||
|
||
static_cast<Magnum::Shaders::Phong&>(*shaderPrograms_[TEXTURED_SHADER]) | ||
.setLightPositions({Magnum::Vector3{10.0f, 10.0f, 10.0f} * 100.0f, |
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.
Are these light positions randomly picked?
My idea is to put the lights on the corners of the bounding box of the scene, which can be adaptive.
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.
Yes, those are just a verbatim copy from magnum-player. There it was hand-tuned to give a nicely balanced lighting (and I think it's not bad here either), but something else could fit here even better.
The lights are currently relative to the camera so you always get the scene uniformly lit -- not sure if making them fully static would achieve the same, maybe balance them out? Definitely might need some fine-tuning :)
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.
The lights are currently relative to the camera so you always get the scene uniformly lit -- not sure if making them fully static would achieve the same, maybe balance them out? Definitely might need some fine-tuning :)
I may misunderstand you:
When the camera moves, the lights follow?
I think it might be more realistic if the lights are fixed (though the scene looks ugly from some angles), especially if we would like to add shadows to the objects in the scene (we talked about it recently in the team meeting).
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.
When the camera moves, the lights follow?
Yes, because that looked like the best tradeoff for me -- as there are no shadows, having a room affected by a light coming through walls from some other room will not look realistic at all.
IIRC back in April we talked about some models having designated light positions that could be used for giving the lights static positions? That could make sense to do together with shadows.
This PR is quite experimental, adding a completely new feature where it's unclear how it should be used. So please clone it, play with it, change light positions / colors / ... to see what fits best. I won't like to merge it without having a consensus on how the visuals should look like :)
009146b
to
826d2cc
Compare
I was looking at Phong.frag and it seem like its using the Phong shading model instead of the half-angle Blinn-Phong shading model. Blinn-Phong is more efficient and what is used by threejs and what was was specified in the original OpenGL fixed function pipeline. I think its also more physically accurate but I have no idea if that's true. shininess values need to be scaled though to get the same look. You may get a performance increase with Blinn-Phong.
https://threejs.org/docs/#api/en/materials/MeshPhongMaterial |
90db441
to
e05f9a5
Compare
PR description updated with something actually presentable. One question remains for this to be actually usable -- how to distinguish what is a mesh that already has shading baked in and what is not ... in the case above I'm branching on whether the mesh has normals (because the room doesn't have but the chairs do), but that of course doesn't work in general (PLYs are usually normal-less, some scans can have normals as well...). Other ideas? cc: @erikwijmans @msavva |
We have in the past talked about adding a |
Just realized -- for glTF that info could be already encoded in the file, since there are extensions like KHR_materials_unlit, but that's just glTF. (And then other stuff like various alternative PBR material specifications etc.) Scans loaded from OBJ / PLY can't really have that kind of info, tho. |
Could we include this functionality as optional and choose a default for now? Maybe we add an option to the SceneConfiguration and then work on another PR with logic to intelligently set that option for our datasets? We could set default to flat shading without breaking any existing expectations and then set phong in places where we have some way to confirm it. For context: I am working on adding primitives to the scene and I'd like to do so with Phong Shader, so this functionality has a purpose beyond the scene itself. It appears that forking on shader type for a scene automatically is the remaining issue and this could be another PR. |
SceneConfiguration ... would mean editing a JSON file? Not a fan of config files :) The main logic that seems to branch over various dataset types is in esp/assets/Asset.cpp, and the Looking there, it seems to pick any GLB file as a MP3D dataset (i.e., unshaded), so that check would need to be extended to verify the GLB file is really from MP3D and not something else. EDIT: today i'm accidentally closing everything, sorry. |
Actually I was suggesting adding an optional flag or list of flags to the SceneConfiguration struct with the default being FLAT. We could pass that through at scene loading time giving the option to setup a phong shaded scene programmatically. This struct is also in the python bindings and used by Simulator class to determine how to load the scene. Shader could be passed to loadScene at that point. Giving the user a way to manually override our automated shader type might be a nice feature anyway for odd use cases we don't test.
Agreed, but this will likely be an ongoing process as we dig into what differentiates various datasets and could be another PR. Similarly, the AssetInfo is loaded in Simulator and if no override flag is present could be used to fork on shader type as we build more aware default behavior. |
Is there any update on this PR? It has been a couple of months since any comments have been made. If it is not ready, I will look into baking in textures on my models, since they look flat when inserted into the scene. |
Last we discussed, @aclegg3 promised to take over. The remaining blocker is deciding when to use flat shading (for photogrammetry scenes with lighting baked in) and when phong (artificial objects, House3D etc.). I don't have enough knowledge about the various datasets Habitat is using to implement this detection & decision making. |
Motivation and Context
before:
after:
This PR depends on #151 (which depends on #150), and only changes use of Flat to Phong, together with adding a fine-tuned light setup. What you don't see here is a completely fresh set of tests for all shader combinations in Magnum itself, which makes adding new features (such as vertex color, object ID output, normal mapping, alpha masking, ...) much easier and less error prone. This will also enables many new features in the near future, such as instanced rendering for physics objects.
How Has This Been Tested
Viewer looks prettier than before, but I think some ground truth data need to be updated for this to pass tests. @erikwijmans could you take care of that? 🙏
Perf measurements -- there's expected slowdown compared to #150 and #151, as the shader is getting much more data and doing much more work now. However it's not too bad I would say -- it will get much worse with PBR: