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

Extend shaders support #24

Open
osrf-migration opened this issue Apr 30, 2018 · 3 comments
Open

Extend shaders support #24

osrf-migration opened this issue Apr 30, 2018 · 3 comments
Labels
enhancement New feature or request

Comments

@osrf-migration
Copy link

Original report (archived issue) by Ian Chen (Bitbucket: Ian Chen, GitHub: iche033).


from comment in pull request #55:

@osrf-migration
Copy link
Author

Original comment by Nate Koenig (Bitbucket: Nathan Koenig).


  • Edited issue description

@osrf-migration osrf-migration added major enhancement New feature or request labels Apr 15, 2020
@chapulina chapulina removed the major label May 25, 2020
@WilliamLewww
Copy link
Contributor

WilliamLewww commented Mar 31, 2021

Reference Branch

fragmentShaderParams Populating

I was digging around the uniform implementation and I noticed that the fragmentShaderParams (in OgreMaterial.cc) are created on demand.

void OgreMaterial::SetFragmentShader(const std::string &_path) in OgreMaterial.cc

  ...
  this->ogreMaterial->compile();
  this->ogreMaterial->load();

  this->fragmentShaderPath = _path;
  this->fragmentShaderParams.reset(new ShaderParams);
}

Since OGRE can query all the shader parameters, should we populate fragmentShaderParams after successfully compiling the shader?

Currently if you try to update a uniform that does not exist it will still be added to the fragmentShaderParams but OGRE will throw an error when trying to update the non-existent uniform.

Example Code:

  ir::ShaderParamsPtr shaderParams = shader->FragmentShaderParams();
  (*shaderParams)["thisUniformDoesNotExist"] = 0;

Output:
terminate call after throwing an instance of 'Ogre::InvalidParametersException' what(): OGRE EXCPETION(2:InvalidParametersException): Parameter called thisUniformDoesNotExist does not exist. in GpuProgramParameters::_findNamedConstantDefinition at /home/wlew/work/ogre-1.9/src/ogre-1.9.1/OgreMain/src/OgreGpuProgramParams.cpp (line 1725)

If we have a pre-existing list of all the uniforms, we could handle the error on ign-rendering rather than having OGRE handle it. Populating the fragmentShaderParams would also let us provide all the available uniforms for a shader program.

Support for 4-byte buffers

I have noticed that OGRE provides the following method which allows setting a uniform buffer of an arbitrary length:

void Ogre::GpuProgramParameters::setNamedConstant(const String& name, const int* val, size_t count, size_t multiple = 4)

I implemented an example on the reference branch that shows updating different uniform buffers with different sizes:

ign-rendering/examples/custom_shaders_uniforms/media/fragment_shader.glsl

uniform float testFloat;
uniform int testInt;
uniform float testFloatBuffer[6];
uniform int testIntBuffer[5];
uniform vec4 testVector4;
uniform mat4 textMatrix4;

void main()
{
  gl_FragColor = vec4(textMatrix4[0][0], textMatrix4[3][1], textMatrix4[3][3], 1.0);
}

ign-rendering/examples/custom_shaders_uniforms/GlutWindow.cc

  ir::ShaderParamsPtr shaderParams = shader->FragmentShaderParams();

  float testMatrix[16] = {
    0.1, 0.2, 0.3, 0.4,
    0.5, 0.6, 0.7, 0.8,
    0.9, 0.1, 0.2, 0.3,
    0.4, 0.5, 0.6, 0.7,
  };
  (*shaderParams)["textMatrix4"].InitializeBuffer(16);
  (*shaderParams)["textMatrix4"].UpdateBuffer(testMatrix, 16);

If we were to populate the fragmentShaderParams after compiling the shader program, the InitializeBuffer method call could be removed from the user application because OGRE can query the size of a buffer.

@chapulina
Copy link
Contributor

If we have a pre-existing list of all the uniforms, we could handle the error on ign-rendering rather than having OGRE handle it.

+1. And I think it may also be a good idea to have a safety catch in case something shows up that we don't know about, so we don't just crash on the user.

Reference Branch

How about opening a pull request so we can iterate on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants