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

Draft Material in core #4

Draft
wants to merge 56 commits into
base: material-in-core
Choose a base branch
from

Conversation

grandch
Copy link

@grandch grandch commented Jun 19, 2023

Pull Request Desription

declare (and implement) bsdf specific interface in material model (according to STORM-IRIT/Radium-Engine/pull/1050 :

bsdf (eval), taking w_i, w_o, N, uv as parameter and return the bsdf value for this configuration
sample : taking w_o, N, uv as parameter and return (w_i, pdf) of the generated direction
pdf , taking w_i, w_o, N, uv as parameter and return the probability of w_i wrt w_o, N, uv

TODO

Move std::minstd_rand randomEngine into a class member to avoid initializing it each call

Move math methods into Math.hpp or LinearAlgebra.hpp

improve class Core::Tex to manage texture, or rely on openimageio

Check if you branch history is PR compatible

  • Your branch need to be up to date with origin/master AND to have linear history (i.e. no merge commit).
  • Update your git repository git fetch origin if origin is this remote
  • Check with the script provided in scripts/is-history-pr-compatible.sh
  • You must use clang-format style
    These checks are enforced by github workflow actions
    Please refer to the corresponding log in case of failure

UPDATE the form below to describe your PR

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Be aware that the PR request cannot be accepted if it doesn't pass the Continuous Integration tests.

  • What kind of change does this PR introduce?

    • bug fix
    • feature
    • docs update
    • other:
  • What is the current behavior? (You can also link to an open issue here)

  • What is the new behavior (if this is a feature change)?

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:

@grandch grandch marked this pull request as draft June 19, 2023 13:15
Copy link
Owner

@MathiasPaulin MathiasPaulin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this first submission. We need to discuss in front of a white board to improve your code and to make the design more efficient.

src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/MaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
@grandch
Copy link
Author

grandch commented Jun 20, 2023

About the random engine. Despite the fact that my getter is bad, David didn't want to have a new random engine for each call. That's why we thought about a static member.

With your solution, do you want to make a new one each time the user doesn't provide a generator ?

@MathiasPaulin
Copy link
Owner

With your solution, do you want to make a new one each time the user doesn't provide a generator ?

No, even if the generators from std:: is very cheap to construct, if the user does not provide a generator, the same will be used for each material.

Even if you have a static member, right now, as your getter returns a copy of the generator, you'll create one each time you call getRandomEngine. You should return a reference in this method, not a value.

What I've in mind is something similar to what I propose (but only for uniform pdf) in the Random package of the (draft) PR Radium dataflow STORM-IRIT#1007.
I just summarize here the expected principle.

We will define a set of Generator classes that generates sequences of dD random numbers (1D, 2D, 3D, ...), uniformly distributed in [0, 1]^d hypercube.

  • exemples : std::mt19937 is such a generator, VanDerCorputSequence is a 1D sequence, Sobol is a multidimensional low discrepancy sequence, etc ...

we use these in a 'Sampler' adapters that generates xD point distributed according to a given probability function

  • exemple, in the above PR, SphericalPointSet generates a direction, with a cosine distribution on the sphere, HammersleyPointSet generates 2D points uniformly distributed on a unit square, in your proposal, the method sampleHemisphereCosineWeighted could be such an adapter ...

In the MaterialModel class, we give the user the ability to set the default 2D Generator (sampling a bsdf need at least 2D random numbers and, if more are needed, the generator will be call more than once for a given sample) to use when sampling, and, if no default is given by the user, we use a std::mt19937 to generate sequence of 2D points (or a simple but better Hamersley2D)

Then the sample method in MaterialModel is overloaded so that the user can call the method with our without a reference to a Generator. If called with such a reference, the given generator will be used by the sampling adapter, if called without, the default will be used by the adapter.

We'll discuss about this on the white board on our monday meeting.

@grandch
Copy link
Author

grandch commented Jun 27, 2023

Yesterday we talked about precomputed tangent and bitangent. I can't access these data from the material (or bsdf) class.
What do you think about keeping all the sampling and bsdf calculations in the local space and letting the caller (the raytracer plugin) that handles the intersection informations, convert the results in the world space ?

@MathiasPaulin
Copy link
Owner

For some materials (e.g. anisotropic materials), accessing the local frame should be mandatory. So, perhaps we can add this local frame as parameter for the bsdf eval, sample and pdf methods.
In the glsl interface for bsdf computation, computing the local frame (and the accessing the normal and tangent) is done in the vertex shader.
Outside glsl interface, e.g. for raytracing, adding the local frame as parameter for the bsdf methods will improve material interface and will allow to compute normal mapping (or displacement mapping in the future) in the base material class.

@dlyr
Copy link

dlyr commented Jun 28, 2023

Should the local frame be the canonical one ? i.e. (1,0,0), (0,1,0), (0,0,1) for textures
Also, as an example in pbrt material evaluation takes the samble interaction that contains uv, du, dv for texture sampling, and own bsdfs

@MathiasPaulin
Copy link
Owner

Should the local frame be the canonical one ? i.e. (1,0,0), (0,1,0), (0,0,1) for textures Also, as an example in pbrt material evaluation takes the samble interaction that contains uv, du, dv for texture sampling, and own bsdfs

Well, My writings seems quite obscure.
Yep, the local frame is always the canonical one.
But how do you go from world (or model) frame to local frame (and vice-versa) ?
You need to know the normal and tangent to build the world-to-local transformation (or model-to local) that will be used to transform incoming directions, expressed in world frame in ray-tracing, to local frame and sampled direction from local frame to global frame.
This is known as TBN frame and its computation is described at (but not only at) https://www.cs.upc.edu/~virtual/G/1.%20Teoria/06.%20Textures/Tangent%20Space%20Calculation.pdf and is already managed when loading meshes. The geometry gives access to normal, tangent (and optionally bitangent) attributes computed this way by loaders (assimp, gltf, ...)

The texture coordinates uv is used to access local bsdf parameter from textures and their derivatives du and dv are used to filter the texture (mip-maping or anisotropic filtering). These derivatives are computed by the ray-tracing engine using ray-differentials (see siggraph '99 paper https://graphics.stanford.edu/papers/trd/ ) and are used in the implementation of the initial ray-tracing plugin.

In pbrt, the sample interaction is a kind of closure that stores partial functions and data that should be computed once per interaction. In this "closure" (see file core/interaction.h in the pbrt source code), you'll find all the informations needed to process an interaction point. This notion of closure is also managed by OpenShadingLangage and in many ray-tracing systems. I have implemented such an approach (first, compute a closure on an interaction point, then use the closure for subsequent computations) in the glsl implementation of GLTF material system.

So right now, and for the work of @grandch, I propose to just take all needed data as parameters of the corresponding function.

On my side (and this will be merged with the current work easily once valldated) I'm experimenting something like :

  • Define a "geometry closure" that embeds all geometric related attributes at an interaction points. The closure is computed by, e.g. the intersector of the ray-tracing system. Such a closure will contains geometric and differential geometric informations such as TBN to go from world to local frame, surface derivative (used to compute ray-differentials), ...
  • Use this "geometry closure" to construct a "bsdf closure" according to a given bsdf model (blinn-phong, microfacet, layered, ...). Such a closure will store normal after normal mapping (for each layer if layered materials) and partially evaluated function on known informations (e.g input direction, bsdf parameters from textures, ...)
  • Use the "bsdf closure" to sample, evaluate, shade, ... at the current intersection point to not compute twice the same information.

@grandch
Copy link
Author

grandch commented Jun 29, 2023

At this point I have a few questions.

I have a ImportanceSampler base class from which derives SphereSampler but I can't figure what interface it should have.

Where does the user provide the random generator ? At the sampler level or at the material/bsdf level ?

Do you have preferences about how you want me to deal with static virtual methods ? I read several workarouds to achieve this.

src/Core/Material/BlinnPhongMaterialModel.hpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/BlinnPhongMaterialModel.cpp Outdated Show resolved Hide resolved
src/Core/Material/SimpleMaterialModel.hpp Outdated Show resolved Hide resolved
~UniformSphereSampler() {};

static std::pair<Vector2, Scalar> getPointImplem( UniformGenerator* generator );
static std::pair<Vector3, Scalar> getDirImplem( UniformGenerator* generator );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static std::pair<Vector3, Scalar> getDirImplem( UniformGenerator* generator );
static std::pair<Vector3, Scalar> getDir( UniformGenerator* generator );

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This getDirImplem method is here for the CRTP SphereSampler in order to have both static methods AND polymorphism.

static std::pair<Vector2, Scalar> getPointImplem( UniformGenerator* generator );
static std::pair<Vector3, Scalar> getDirImplem( UniformGenerator* generator );

static Scalar pdfImplem( Vector3 dir, Vector3 normal );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static Scalar pdfImplem( Vector3 dir, Vector3 normal );
static Scalar pdf( Vector3 dir, Vector3 normal );

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pdfImplem method is here for the CRTP SphereSampler in order to have both static methods AND polymorphism.

src/Core/Random/UniformSphereSampler.hpp Outdated Show resolved Hide resolved
src/Core/Material/MaterialModel.hpp Outdated Show resolved Hide resolved
src/Core/Material/MaterialModel.hpp Outdated Show resolved Hide resolved
@grandch
Copy link
Author

grandch commented Aug 3, 2023

Here are two renders I did with the radium-embree path-tracer plugin. All these renders use the methods I wrote here in core materials.

The first one took 64 samples and one bounce max.

64samples1bounce

The second one has the same parameters but only display de specular reflections.

specularOnly

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.

3 participants