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

Heightmap for Ogre 1 #180

Merged
merged 36 commits into from
Jan 30, 2021
Merged

Heightmap for Ogre 1 #180

merged 36 commits into from
Jan 30, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Nov 25, 2020

This is a port of Gazebo classic's heightmap implementation.

  • New generic APIs so any render engine can implement heightmaps
  • Implementation for Ogre 1 with tests
  • Example
  • Tutorial

I'm testing this locally with Ogre 1.9.1.

image

Update: Also got it to work with 1.9.0 (debs or source with this patch)

Implementation considerations / questions

Geometry vs Object

My initial implementation had heightmaps as a new geometry type that can be attached to visuals. That's different from Gazebo classic, but it matches the SDF spec.

However, Ogre terrains don't seem to have an Ogre::MovableObject, which is required for OgreGeometrys. Looking online, it doesn't seem like terrains are meant to be attached to nodes and moved around. So I stopped pursuing that direction, but left it on branch chapulina/5/heightmap. I don't think we should make decisions about ign-rendering's API based solely on Ogre though, so I'm open to bringing that back if we think that it will fit well with other engines.

For now, heightmaps are extending Object, which seemed to be the correct class for entities that won't be moving too much.

Heightmaps are now a Geometry, see conversation below.

Multiple heightmaps

The current implementation leaves room for loading multiple heightmaps at the same time. It currently loads the geometry well, but the materials can't be loaded yet. I'm planning to keep looking into fixing that.

Multiple heightmaps are now working, see example.

Scene API

The only function added to Scene so far was CreateHeightmap and it's storing the heightmaps in a simple vector. I didn't want to go too far until we ruled out the geometry approach. If we keep heightmaps as objects that are managed by the scene, I think we should:

  • Create a HeightmapStorage class
  • Add HasHeightmap* / HeightmapBy* / HeightmapCount / DestroyHeightmap APIs to Scene

I can do that as part of this PR if we want to take this direction.

Punting on this now that heightmaps are geometries.

Descriptor

I took some inspiration from MeshDescriptor and stored all the information for the heightmap into HeightmapDescriptor. Some key differences:

  • HeightmapDescriptor is a first-class element of Heightmap, while Mesh doesn't know about MeshDescriptor.
  • Heightmap stores a copy of HeightmapDescriptor and exposes that through the Descriptor() function. That's different to storing and accessing each property separately. If this is an acceptable pattern, I can look into PIMPLizing it so it can be extended in the future.

Material

Admittedly, I blindly copied the terrain material code from Gazebo classic and haven't looked closely into it yet. There may be potential for using ignition::rendering::Material inside Heightmap, but judging by all the custom TerrainMaterial code, that may not be possible or desirable. I just wanted to make sure I brought this up.

Functionality

  • Multiple textures with blending
  • Paging
  • Caching - not working yet. Saving seems to work, but loading isn't working yet.
  • Loading multiple heightmaps - material is broken

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added enhancement New feature or request close the gap Features from Gazebo-classic labels Nov 25, 2020
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Nov 25, 2020
@osrf-triage osrf-triage added this to Inbox in Core development Nov 25, 2020
@chapulina chapulina moved this from Inbox to In progress in Core development Nov 25, 2020
@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #180 (2a4ebda) into main (aee4a7d) will increase coverage by 4.28%.
The diff coverage is 74.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #180      +/-   ##
==========================================
+ Coverage   53.01%   57.29%   +4.28%     
==========================================
  Files         143      147       +4     
  Lines       13600    14769    +1169     
==========================================
+ Hits         7210     8462    +1252     
+ Misses       6390     6307      -83     
Impacted Files Coverage Δ
include/ignition/rendering/Scene.hh 0.00% <ø> (ø)
include/ignition/rendering/base/BaseScene.hh 0.00% <ø> (ø)
ogre/src/OgreCamera.cc 0.00% <0.00%> (ø)
ogre2/src/Ogre2Scene.cc 88.51% <0.00%> (-1.93%) ⬇️
ogre/src/OgreVisual.cc 27.55% <28.57%> (+27.55%) ⬆️
ogre/src/OgreHeightmap.cc 72.70% <72.70%> (ø)
include/ignition/rendering/base/BaseHeightmap.hh 72.72% <72.72%> (ø)
ogre/src/OgreScene.cc 28.88% <80.00%> (+28.88%) ⬆️
src/HeightmapDescriptor.cc 98.33% <98.33%> (ø)
include/ignition/rendering/HeightmapDescriptor.hh 100.00% <100.00%> (ø)
... and 17 more

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 aee4a7d...5c423b3. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Dec 15, 2020

Geometry vs Object: looking at threejs as another example, it treats heightmap as a geometry. I think semantically it makes sense. The OgreObject() will then just return null. We may have to update OgreVisual::AttachGeometry function to work with this special case of a null obj. The other option is to derive the Heightmap class from Visual, so when the user calls CreateHeightmap, it's creating the node and the geometry at the same time. I'm leaning towards the Geometry approach but not strongly attached to it.

Multiple heightmaps: sounds good

Scene API: If it's derived from Geometry, we could create a general GeometryStorage class. It looks like currently other geometries (e.g. text and markers, etc) are not stored in a storage. So we could start storing them and offer APIs for retrieving and destroying geometries. This can be ticketed as an issue and addressed separately. If Heightmap is derived from Visual, then we can just call Scene::RegisterVisual to store it in the visual store

Descriptor: HeightmapDescriptor class is a good idea

Material ogre heightmap material is tricky. It may not be trivial to make it work with a generic rendering::Material yet. I'm leaning towards not offering the API for users to set heightmap material for now.

@chapulina
Copy link
Contributor Author

Thanks for the input, @iche033 . I'll revert to inheriting from Geometry and fix whatever is needed from there. While I'm working on the ign-gazebo integration, I'm also seeing that approach should make things easier there.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
…forced

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
….9.0 but not 1.9.1

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina moved this from In progress to In review in Core development Dec 21, 2020
@iche033
Copy link
Contributor

iche033 commented Dec 22, 2020

I'm getting a couple of compile errors, which are also happening on jenkins:

/home/osrf/code/ign_d_ws2/src/ign-rendering/ogre/src/OgreHeightmap.cc:1503:1: error: ‘class IgnTerrainMatGen::SM2Profile::ShaderHelperGLSL’ is protected within this context
 IgnTerrainMatGen::SM2Profile::ShaderHelperGLSL::

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

compile errors

Ahh sorry, it should be fixed now: b3efe1c

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.

works for me! Did a first pass and left some minor comments

include/ignition/rendering/HeightmapDescriptor.hh Outdated Show resolved Hide resolved
ogre/src/OgreHeightmap.cc Outdated Show resolved Hide resolved
ogre/src/OgreHeightmap.cc Outdated Show resolved Hide resolved
src/HeightmapDescriptor.cc Outdated Show resolved Hide resolved
ogre/src/OgreHeightmap.cc Outdated Show resolved Hide resolved
ogre/include/ignition/rendering/ogre/OgreHeightmap.hh Outdated Show resolved Hide resolved
ogre/src/OgreHeightmap.cc Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@iche033
Copy link
Contributor

iche033 commented Jan 23, 2021

@osrf-jenkins run tests please

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.

looks good! waiting for one more round of CI builds

tutorials/21_heightmap.md Outdated Show resolved Hide resolved
include/ignition/rendering/HeightmapDescriptor.hh Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
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.

I see lots of greens!

@chapulina chapulina merged commit 1dbdef4 into main Jan 30, 2021
Core development automation moved this from In review to Done Jan 30, 2021
@chapulina chapulina deleted the chapulina/5/heightmap_single branch January 30, 2021 02:34
@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
close the gap Features from Gazebo-classic 🏢 edifice Ignition Edifice enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants