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

Use Phong shader for rendering scenes #152

Closed
wants to merge 1 commit into from
Closed

Conversation

mosra
Copy link
Collaborator

@mosra mosra commented Aug 14, 2019

Motivation and Context

before:

chair3-before

after:

chair3-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:

image

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 14, 2019
using namespace Magnum::Math::Literals;

static_cast<Magnum::Shaders::Phong&>(*shaderPrograms_[TEXTURED_SHADER])
.setLightPositions({Magnum::Vector3{10.0f, 10.0f, 10.0f} * 100.0f,
Copy link
Contributor

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.

Copy link
Collaborator Author

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 :)

Copy link
Contributor

@bigbike bigbike Aug 15, 2019

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).

Copy link
Collaborator Author

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 :)

@mosra mosra force-pushed the magnum-flat-shader branch 3 times, most recently from 009146b to 826d2cc Compare August 16, 2019 17:04
@msbaines
Copy link
Contributor

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.

        /* Add specular color, if needed */
        if(intensity > 0.001) {
            highp vec3 reflection = reflect(-normalizedLightDirection, normalize\
dTransformedNormal);
            mediump float specularity = clamp(pow(max(0.0, dot(normalize(cameraD\
irection), reflection)), shininess), 0.0, 1.0);
            fragmentColor += vec4(finalSpecularColor.rgb*specularity, finalSpecu\
larColor.a);
        }

https://threejs.org/docs/#api/en/materials/MeshPhongMaterial
https://www.khronos.org/registry/OpenGL/specs/gl/glspec21.pdf

@mosra mosra changed the base branch from magnum-flat-shader to master August 19, 2019 10:44
@mosra mosra force-pushed the magnum-phong-shader branch from 90db441 to e05f9a5 Compare August 19, 2019 13:25
@mosra mosra mentioned this pull request Aug 31, 2019
60 tasks
@mosra
Copy link
Collaborator Author

mosra commented Sep 19, 2019

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

@erikwijmans
Copy link
Contributor

We have in the past talked about adding a habitat_config.json to the root directory for various datasets to specify things like the gravity direction. We could revive that idea to specify whether or not to use phong?

@mosra
Copy link
Collaborator Author

mosra commented Sep 19, 2019

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.

@aclegg3
Copy link
Contributor

aclegg3 commented Sep 25, 2019

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.

@mosra
Copy link
Collaborator Author

mosra commented Sep 25, 2019

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 AssetType enum in particular, right? So there it could be extended to detect whether it needs to render flat or shaded, and pick a shader type based on that.

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.

@mosra mosra closed this Sep 25, 2019
@mosra mosra reopened this Sep 25, 2019
@aclegg3
Copy link
Contributor

aclegg3 commented Sep 25, 2019

SceneConfiguration ... would mean editing a JSON file? Not a fan of config files :)

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.

The main logic that seems to branch over various dataset types is in esp/assets/Asset.cpp, and the AssetType enum in particular, right? So there it could be extended to detect whether it needs to render flat or shaded, and pick a shader type based on that.

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.

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.

@dhuber23b
Copy link

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.

@dhuber23b
Copy link

Here's an example. Notice that the chair has no shading.
077

@mosra
Copy link
Collaborator Author

mosra commented Nov 21, 2019

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.

@aclegg3
Copy link
Contributor

aclegg3 commented Feb 25, 2020

Closing this based on other recent progress. (#500, #489)

@aclegg3 aclegg3 closed this Feb 25, 2020
@mosra mosra deleted the magnum-phong-shader branch February 26, 2020 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants