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

Add waves #541

Merged
merged 6 commits into from
Jan 28, 2022
Merged

Add waves #541

merged 6 commits into from
Jan 28, 2022

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Jan 21, 2022

Signed-off-by: Ian Chen ichen@osrfoundation.org

🎉 New feature

Summary

Added a waves demo that makes use of custom shaders.

Extended ShaderParams to support texture types (2d and cube map) so that users can pass textures to shaders.

Note: I took the shaders code from VRX, tweaked and simplified them a little for this demo. I left the copyright notices intact. Looks like @srmainwaring is one of the original authors.

Test it

Build and run the waves example:

waves

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@osrf-triage osrf-triage added this to Inbox in Core development Jan 21, 2022
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Jan 21, 2022
@chapulina chapulina moved this from Inbox to In review in Core development Jan 21, 2022
Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #541 (56309b1) into ign-rendering6 (fae20cd) will decrease coverage by 0.08%.
The diff coverage is 18.75%.

Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering6     #541      +/-   ##
==================================================
- Coverage           54.47%   54.38%   -0.09%     
==================================================
  Files                 198      198              
  Lines               20099    20147      +48     
==================================================
+ Hits                10948    10957       +9     
- Misses               9151     9190      +39     
Impacted Files Coverage Δ
ogre2/src/Ogre2Material.cc 65.73% <0.00%> (-5.15%) ⬇️
src/ShaderParam.cc 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fae20cd...56309b1. Read the comment docs.

@srmainwaring
Copy link
Contributor

Nice @iche033! The original concept for the ocean shaders comes from this article https://developer.nvidia.com/gpugems/gpugems/part-i-natural-effects/chapter-1-effective-water-simulation-physical-models and was implemented in the demos for Ogre (IIRC) and then in one of the examples in https://uuvsimulator.github.io/ (visuals only). There was some work with the VRX team to sync up the physics sim with the visuals (so boats would rise at the same time as the shader visuals moved the ocean).

For Gazebo classic I developed a more involved ocean simulation here https://github.com/srmainwaring/asv_wave_sim/tree/feature/fft_waves, which I've been meaning to port to Ignition. I think the hydrodynamics part has too many dependencies on libraries under GPL to submit a PR to Ignition, but the FFT ocean surface generator may be suitable for inclusion if there was interest.

@iche033
Copy link
Contributor Author

iche033 commented Jan 25, 2022

@srmainwaring , cool thanks for the links and info!

I added a reference to the nvidia water simulation page in the shader code.

There was some work with the VRX team to sync up the physics sim with the visuals (so boats would rise at the same time as the shader visuals moved the ocean).

yeah we will be porting the wave field plugins over to Ignition too and also sync physics and the visuals.

For Gazebo classic I developed a more involved ocean simulation here https://github.com/srmainwaring/asv_wave_sim/tree/feature/fft_waves,

Nice! FFT based ocean waves will be great. From a quick look at the repo, I see that the wave simulation code depends on fftw which is GPL. What's the "FFT ocean surface generator"? Is that the part that depends on CGAL for generating meshes?

@srmainwaring
Copy link
Contributor

What's the "FFT ocean surface generator"? Is that the part that depends on CGAL for generating meshes?

The bit that depends on CGAL is the hydrodynamics plugin. There are two what ignition calls systems.

The first system generates surface waves in dynamically generated mesh. The FFT wave generator uses a FFT to create a wave spectrum - I've used fftw to implement, but another library could be used. There's also an OpenCL implementation included (but I might run into macOS unfriendliness with that as well in future).

The second system, hydrodynamics, calculates the pressure and drag forces on each triangle in the mesh using a technique described by one of the FarCry devs . It's an alternative to estimating volumes and calculating buoyancy that way. The advantage with the surface pressure approach is that it automatically determines which portion of the craft is submerged, and when there are waves it accounts for that as well (so for instance you may have the water level higher on one side of the vessel - which leads to a horizontal pressure differential). I used CGAL for ray intersection and a couple of other things - bounding boxes IIRC. That's a bit (lot?) more work to extract, but the whole calc should be parallelised anyway and would be improved with a rework.

@iche033
Copy link
Contributor Author

iche033 commented Jan 27, 2022

I see thanks for the explanation! We'll likely not be able to depend on fftw due to its license so will need an alternative. A quick search yields this header only library. There could be others.

The second system, hydrodynamics, calculates the pressure and drag forces on each triangle in the mesh using a technique described by one of the FarCry devs .

interesting, that sounds like it produces a lot more accurate motion of the surface vehicle, like waves hitting the side of the boat.

I see the gif in your repo illustrating the more accurate behavior of objects floating on the waves. Nice.

@arjo129
Copy link

arjo129 commented Jan 28, 2022

The second system, hydrodynamics, calculates the pressure and drag forces on each triangle in the mesh using a technique described by one of the FarCry devs . It's an alternative to estimating volumes and calculating buoyancy that way. The advantage with the surface pressure approach is that it automatically determines which portion of the craft is submerged, and when there are waves it accounts for that as well (so for instance you may have the water level higher on one side of the vessel - which leads to a horizontal pressure differential). I used CGAL for ray intersection and a couple of other things - bounding boxes IIRC. That's a bit (lot?) more work to extract, but the whole calc should be parallelised anyway and would be improved with a rework.

Hey @srmainwaring this sounds quite interesting currently we do have a linear hydrodynamics plugin ported from the UUV sim that is suited for underwater vessels upstream in ignition. The approach you use does sound interesting as it means we don't need to calculate numbers for Added Mass and Drag Coefficients which are really hard to calculate. By any chance are you aware of the proposal for a maritime working group here? I think it'd be great to have your input in the said group.

Copy link
Contributor

@WilliamLewww WilliamLewww left a comment

Choose a reason for hiding this comment

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

Works and looks amazing!

endif()
endif()

configure_file (example_config.hh.in ${PROJECT_BINARY_DIR}/example_config.hh)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting a compilation error from missing the example_config.hh.in in the waves example directory and I fixed it temporarily by just taking the file from the other examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, added file in 56309b1

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@iche033 iche033 merged commit 973f516 into ign-rendering6 Jan 28, 2022
@iche033 iche033 deleted the waves branch January 28, 2022 23:38
Core development automation moved this from In review to Done Jan 28, 2022
srmainwaring pushed a commit to srmainwaring/gz-rendering that referenced this pull request Feb 3, 2022
* add waves

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* style

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* add link to nvidia water simulation page

Signed-off-by: Ian Chen <ichen@osrfoundation.org>

* add missing example config hh file

Signed-off-by: Ian Chen <ichen@osrfoundation.org>
@srmainwaring
Copy link
Contributor

@iche033 I've made a start porting this and the custom_shaders_uniforms examples to Metal: https://github.com/srmainwaring/ign-rendering/tree/feature/ign-rendering7-metal-waves

Aside from the Metal version of the shaders, a change to Ogre2Material is needed load the Metal shaders when using that graphics API. We'll need a way to select the graphics API in SetVertexShader and SetFragmentShader. The fragment program also needs an additional parameter shader_reflection_pair_hint in the case of Metal otherwise the uniforms are not handled correctly.

The missing piece is how to pass the textures: Metal expects the textures and samplers to be supplied in the argument list of metal_main() rather than being sent with the parameters where the rest of the uniforms are passed. I'd appreciate any insight you or @darksylinc might have on the best way to approach this in a general way that works for GL3+ and Metal.

@darksylinc
Copy link
Contributor

The missing piece is how to pass the textures: Metal expects the textures and samplers to be supplied in the argument list of metal_main() rather than being sent with the parameters where the rest of the uniforms are passed. I'd appreciate any insight you or @darksylinc might have on the best way to approach this in a general way that works for GL3+ and Metal.

The way we handle it is just silly, using macros:

// Shared code;
void myFunction( float a PARAMS_ARG_DECL )
{
  ...
}

myFunction( 1.0f PARAMS_ARG );

// On GLSL
#define PARAMS_ARG_DECL
#define PARAMS_ARG
// This is a uniform
#define p_bandPower			bandMaskPower

// On Metal
#define PARAMS_ARG_DECL , constant Params &p, texture2d<float> myTexture
#define PARAMS_ARG , p, myTexture
// This is a uniform
#define p_bandPower			p.bandMaskPower

Whether this is a net win or net loss depends on the size of the shader. Small shaders it's not worth it (unless they're constantly being modified, since shared code needs to be modified for every API). Bigger ones it is worth it.

@iche033
Copy link
Contributor Author

iche033 commented Feb 3, 2022

cool, thanks for porting the waves shaders to Metal!

As for the code inside ign-rendering for setting up textures to be used in shaders, I thought just creating the texture unit would be sufficient:
https://github.com/ignitionrobotics/ign-rendering/blob/ign-rendering6/ogre2/src/Ogre2Material.cc#L719-L756

Looking at an example material file in ogre-next, GLSL requires passing the textures as params, while I don't see that being done in Metal.

So the difference I see is that this call in ign-rendering may not be needed?
https://github.com/ignitionrobotics/ign-rendering/blob/ign-rendering6/ogre2/src/Ogre2Material.cc#L756

@srmainwaring
Copy link
Contributor

srmainwaring commented Feb 3, 2022

Thanks @darksylinc and @iche033 - very helpful.

Suppressing setting the named constant for textures when using Metal did the trick for the bump map

https://github.com/ignitionrobotics/ign-rendering/blob/ign-rendering6/ogre2/src/Ogre2Material.cc#L756

waves-metal-2-textures

Update

When the cubeMap texture is added to the framentShader params I am seeing odd behaviour in the camera->Capture: deep down the render stack in HmlsLowLevel::executeCommand there is a call to bindGpuProgramParameters. The problem is that the mRenderSystem::mPso is a null pointer and so we get a segfault. Now mPso was valid during camera initialisation (before the uniforms are initialised), but it is invalid later on.

@iche033
Copy link
Contributor Author

iche033 commented Feb 4, 2022

hmm do you know if cube map is the issue or if it's a problem passing multiple textures? e.g. what if you pass just the cube map and no bump map?

@srmainwaring
Copy link
Contributor

I think it's the cubemap. With just the cubemap there is a segfault (null pipeline state object at the binding point again). With just the bump map its ok. I can add multiple 2D textures, although there is an issue that it's unclear which texture unit will be assigned to each texture (it's not necessarily the order in which the texture are added to the parameters)

I've added some debug lines in Ogre2Material to trace:

Added resource location '/Users/rhys/Code/osrf/ign_fortress_ws/src/ign-rendering/examples/waves/build/media/' of type 'FileSystem' to resource group 'General'
[Dbg] [Ogre2Material.cc:713] texUnit.name:     waterTex
[Dbg] [Ogre2Material.cc:714] texUnit.texIndex: 0
Texture memory budget exceeded. Stalling GPU.
Stalling was not enough. Freeing memory.
Texture memory budget exceeded. Stalling GPU.
Stalling was not enough. Freeing memory.
[Dbg] [Ogre2Material.cc:713] texUnit.name:     bumpMap
[Dbg] [Ogre2Material.cc:714] texUnit.texIndex: 1

which means I need to line up the texture arguments to the Metal fragment shader as:

fragment float4 main_metal
(
  PS_INPUT inPs [[stage_in]],
  texture2d<float>    waterTex        [[texture(0)]],
  texture2d<float>    bumpMap         [[texture(1)]],
  texturecube<float>  cubeMap         [[texture(2)]],
  sampler             waterTexSampler [[sampler(0)]],
  sampler             bumpMapSampler  [[sampler(1)]],
  sampler             cubeMapSampler  [[sampler(2)]],
  constant Params &p [[buffer(PARAMETER_SLOT)]]
)

It would be good to be able to select the texture unit when setting custom textures on the shader params:

  std::string waterTexPath = ignition::common::joinPaths(RESOURCE_PATH,
      "water.jpg");
  (*g_fsParams)["waterTex"].SetTexture(waterTexPath);

Here's the result of sampling a water texture and the bump map, no cube map.

waves-metal-3-textures

@srmainwaring
Copy link
Contributor

It's working!

waves-metal-4-textures

I tracked down a difference in OgreTextureUnitState between the way that Ogre::TextureTypes::Type2D and Ogre::TextureTypes::TypeCube are handled and it comes down to this test which is only applied for Type2D:

            // Load immediately ?
            if (isLoaded())
            {
                _load(); // reload
            }

and it turns out if you add the same test in Ogre2Material when setting the cubic texture on the texUnitState then it works:

        texUnit->setCubicTextureName(baseName, true);
        if (texUnit->isLoaded())
        {
          texUnit->_load();
        }

Now I still really don't understand how this manages to trigger setting the mPso in the mRenderSystem variable held by the RenderQueue, perhaps it's something to do with the HlmsCache being updated? @darksylinc may be able to shed some light on what's going on.

Anyway, the good news is that the two required functions are public, so no API changes are needed (or Ogre release).

I've currently rebased the changes for the waves on draft PR #554 which deals with most of the OgreMaterial changes, so will submit a PR for the waves when that's ready to merge.

@iche033
Copy link
Contributor Author

iche033 commented Feb 4, 2022

nice! not very trivial given the backtrace. Glad to see it's working!

@srmainwaring
Copy link
Contributor

not very trivial given the backtrace.

Yeah, that part of the Ogre code down in the depths of the compositor is very complicated to follow - I'd almost given up (and was going to cheat by just using another 2D texture to mock up the reflection effects). I think this combined with your gazebo visual plugins PR provides the pieces I need to get my FFT waves plugin ported - so happy with that!

@darksylinc
Copy link
Contributor

darksylinc commented Feb 4, 2022

Now I still really don't understand how this manages to trigger setting the mPso in the mRenderSystem variable held by the RenderQueue, perhaps it's something to do with the HlmsCache being updated? @darksylinc may be able to shed some light on what's going on.

Low level materials are full of legacy code and behaviors (and a few "wtf were they thinking?"). Since ign-rendering decided to fully construct a low level material and its shaders via code, you're encountering some of the ugly sides.

We're keeping low level materials because:

  1. They make porting easier
  2. They're user friendly for specific stuff (e.g. compositor FXs)
  3. They're very useful for specific scenarios (like rendering a particular object in a special way)
  4. Most of them are defined via scripts so Ogre hides the esoteric calls needed to get it working correctly (like calling setCubicTextureName)

Oh I remembered something: Cubemaps were treated specially in low level materials because there were two ways to load them:

  1. The file natively stores a cubemap (e.g. DDS)
  2. The cubemap is madeup of 6 files so they're specified individually but loaded together to form a cubemap. That partially explains why it got special treatment.

Now I still really don't understand how this manages to trigger setting the mPso in the mRenderSystem variable held by the RenderQueue, perhaps it's something to do with the HlmsCache being updated?

Care to elaborate the actual code locations and call stacks?

@srmainwaring
Copy link
Contributor

srmainwaring commented Feb 5, 2022

Sure, this is the function in OgreTextureUnitState where Type2D gets the reload treatment

https://github.com/OGRECave/ogre-next/blob/f8f59465a55d7cb812b8a568b913dad77890ef4a/OgreMain/src/OgreTextureUnitState.cpp#L196-L228

This is the call in OgreHlmsLowLevel.cpp just prior to the segfault

https://github.com/OGRECave/ogre-next/blob/f8f59465a55d7cb812b8a568b913dad77890ef4a/OgreMain/src/OgreHlmsLowLevel.cpp#L344-L348

which occurs here

https://github.com/OGRECave/ogre-next/blob/f8f59465a55d7cb812b8a568b913dad77890ef4a/RenderSystems/Metal/src/OgreMetalRenderSystem.mm#L2516-L2526

because mPso is null.

I went searching for where mPso was initialised, and it's retrieved from the HlmsCache before being bound into a CommandBuffer. I'm wondering if there are some paths that can cause this step to be skipped, and perhaps forcing the reload invalidated the hashes somewhere which forces the cache to be checked again.

@darksylinc
Copy link
Contributor

The relevant code that sets the PSO is:

Hlms *hlms = mHlmsManager->getHlms( static_cast<HlmsTypes>( datablock->mType ) );

lastHlmsCacheHash = lastHlmsCache->hash;
const HlmsCache *hlmsCache = hlms->getMaterial( lastHlmsCache, passCache[datablock->mType],
                                                queuedRenderable, casterPass );
if( lastHlmsCache != hlmsCache )
{
    CbPipelineStateObject *psoCmd = mCommandBuffer->addCommand<CbPipelineStateObject>();
    *psoCmd = CbPipelineStateObject( &hlmsCache->pso ); //// <<<-------- here
    lastHlmsCache = hlmsCache;

    // Flush the RenderOp when changing shaders. Needed by D3D11/12 & possibly Vulkan
    lastRenderOp.vertexData = 0;
    lastRenderOp.indexData = 0;
}

uint32 baseInstance = hlms->fillBuffersForV1( hlmsCache, queuedRenderable, casterPass,
                                                          lastHlmsCacheHash, mCommandBuffer );

I suppose a bogus "if( lastHlmsCache != hlmsCache )" could cause the command to be skipped entirely.
But it's also possible hlmsCache->pso->rsData or MetalHlmsPso( hlmsCache->pso->rsData )->pso is nullptr.

If the latter is the case, then something went wrong in HlmsLowLevel::createShaderCacheEntry

There should be errors in the Ogre.log if the PSO failed to be created (which is possible!) while both shaders actually compiled fine.
Try seeing if inside MetalRenderSystem::_hlmsPipelineStateObjectCreated enters the exception:

NSError *error = NULL;
id<MTLRenderPipelineState> pso =
	[mActiveDevice->mDevice newRenderPipelineStateWithDescriptor:psd error:&error];

if( !pso || error )
{
	String errorDesc;
	if( error )
		errorDesc = error.localizedDescription.UTF8String;

	OGRE_EXCEPT( Exception::ERR_RENDERINGAPI_ERROR,
				 "Failed to created pipeline state for rendering, error " + errorDesc,
				 "MetalRenderSystem::_hlmsPipelineStateObjectCreated" );
}

@srmainwaring srmainwaring mentioned this pull request Feb 7, 2022
8 tasks
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-01-citadel-edifice-fortress/1313/1

@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
🏯 fortress Ignition Fortress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants