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

[OGRE 1.x] Uniform buffer shader support #294

Merged
merged 27 commits into from
May 15, 2021
Merged

[OGRE 1.x] Uniform buffer shader support #294

merged 27 commits into from
May 15, 2021

Conversation

WilliamLewww
Copy link
Contributor

@WilliamLewww WilliamLewww commented Mar 31, 2021

Uniform Buffer Shader Support

Related Issue #24

Summary

Currently only INT and FLOAT uniforms are encoded with ShaderParam. The new uniform buffers take advantage of OGRE's following method:

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

Any uniform buffer with a type size of 4 bytes will be available to update and use in a shader program.

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

Test it

Build and run the example custom_shaders_uniforms
uniform

To-do

  • Remove raw pointers for host uniform buffers

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)

@osrf-triage osrf-triage added this to Inbox in Core development Mar 31, 2021
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Mar 31, 2021
@WilliamLewww WilliamLewww changed the title Wlew/extend shader support Extend shader support Mar 31, 2021
@WilliamLewww WilliamLewww changed the title Extend shader support Uniform buffer shader support Mar 31, 2021
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: WilliamLewww <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@chapulina chapulina moved this from Inbox to In progress in Core development Mar 31, 2021
@chapulina chapulina added the enhancement New feature or request label Mar 31, 2021
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

Some style issues

examples/custom_shaders_uniforms/GlutWindow.cc Outdated Show resolved Hide resolved
examples/custom_shaders_uniforms/GlutWindow.cc Outdated Show resolved Hide resolved
examples/custom_shaders_uniforms/GlutWindow.cc Outdated Show resolved Hide resolved
examples/custom_shaders_uniforms/GlutWindow.hh Outdated Show resolved Hide resolved
src/ShaderParam_TEST.cc Outdated Show resolved Hide resolved
src/ShaderParam_TEST.cc Outdated Show resolved Hide resolved
src/ShaderParam_TEST.cc Outdated Show resolved Hide resolved
src/ShaderParam_TEST.cc Outdated Show resolved Hide resolved
src/ShaderParam_TEST.cc Outdated Show resolved Hide resolved
Core development automation moved this from In progress to In review Apr 1, 2021
Co-authored-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

nice demo. I have a few style suggestions.

ogre works fine but I get a crash with ogre2 though

examples/custom_shaders_uniforms/Main.cc Outdated Show resolved Hide resolved
examples/custom_shaders_uniforms/Main.cc Outdated Show resolved Hide resolved
examples/custom_shaders_uniforms/Main.cc Outdated Show resolved Hide resolved
ogre/src/OgreMaterial.cc Outdated Show resolved Hide resolved
src/ShaderParam.cc Show resolved Hide resolved
src/ShaderParam_TEST.cc Outdated Show resolved Hide resolved
examples/custom_shaders_uniforms/GlutWindow.cc Outdated Show resolved Hide resolved
include/ignition/rendering/ShaderParam.hh Outdated Show resolved Hide resolved
include/ignition/rendering/ShaderParam.hh Outdated Show resolved Hide resolved
include/ignition/rendering/ShaderParam.hh Outdated Show resolved Hide resolved
@WilliamLewww
Copy link
Contributor Author

nice demo. I have a few style suggestions.

ogre works fine but I get a crash with ogre2 though

I only worked with the ogre1 files so far because the original custom_shaders example crashes with ogre2 for me. I'm going to dig around the files to see if I can fix the problem or if the problem only exists on my machine.

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@iche033
Copy link
Contributor

iche033 commented Apr 1, 2021

I'm going to dig around the files to see if I can fix the problem or if the problem only exists on my machine.

ok thanks. If it's not trivial to fix, we can ticket an issue first. But I think we should at least print an error msg and have the demo exit gracefully.

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@chapulina chapulina moved this from In review to In progress in Core development Apr 5, 2021
@WilliamLewww WilliamLewww changed the title Uniform buffer shader support [OGRE 1.x] Uniform buffer shader support Apr 19, 2021
@WilliamLewww WilliamLewww marked this pull request as ready for review April 19, 2021 16:20
@WilliamLewww
Copy link
Contributor Author

Implementing uniform buffers in OGRE2 requires a little more work than expected.

Currently only the PBS Hlms is implemented but custom shaders require the LowLevel Hlms implementation.

https://github.com/ignitionrobotics/ign-rendering/blob/a2c172a234ba3ce8b66223fda6197c8778322f57/ogre2/src/Ogre2Material.cc#L531-L535

https://github.com/ignitionrobotics/ign-rendering/blob/a2c172a234ba3ce8b66223fda6197c8778322f57/ogre2/include/ignition/rendering/ogre2/Ogre2Material.hh#L223-L224

I will see if I can implement the two other Hlms (Unlit and LowLevel), but they would probably need their own separate pull request.

Core development automation moved this from In progress to In review Apr 20, 2021
Copy link
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

The Windows PR is yet a draft but I think it makes sense to include here at least the right variables and includes.

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
…ign-rendering into wlew/extend_shader_support
@chapulina chapulina requested review from iche033 and ahcorde May 10, 2021 18:38
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

There are some code check errors. Can you run sh tools/code_check.sh from the ign-rendering dir and and address the issues related to your changes?

Signed-off-by: William Lew <WilliamMilesLew@gmail.com>
@iche033 iche033 merged commit fdc3e5a into gazebosim:ign-rendering4 May 15, 2021
Core development automation moved this from In review to Done May 15, 2021
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants