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

Per-point colors in point clouds #494

Merged
merged 3 commits into from
Dec 7, 2021
Merged

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Nov 24, 2021

🎉 New feature

Summary

We have been populating the colors vector in Ogre2DynamicRenderable, but not using the colors for anything.

For most dynamic renderables, we fill the vertex buffer with position and normal. But points don't have normals, so we've been leaving half the buffer empty. The implementation on this PR takes advantage of those empty slots to place per-point colors (only RGB, no A).

Thanks @WilliamLewww for helping me out with this!

Test it

The marker example on gazebosim/gz-gui#318 uses this:

pts_color

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

@chapulina chapulina added the enhancement New feature or request label Nov 24, 2021
@github-actions github-actions bot added the 🏯 fortress Ignition Fortress label Nov 24, 2021
@codecov
Copy link

codecov bot commented Nov 24, 2021

Codecov Report

Merging #494 (2eb00dd) into main (8143932) will increase coverage by 1.93%.
The diff coverage is 63.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   52.22%   54.16%   +1.93%     
==========================================
  Files         203      203              
  Lines       20215    20227      +12     
==========================================
+ Hits        10557    10955     +398     
+ Misses       9658     9272     -386     
Impacted Files Coverage Δ
ogre2/src/Ogre2Camera.cc 84.84% <ø> (+13.33%) ⬆️
ogre2/src/Ogre2Marker.cc 41.93% <ø> (+1.05%) ⬆️
ogre2/src/Ogre2RayQuery.cc 72.54% <0.00%> (+31.54%) ⬆️
src/Utils.cc 98.70% <ø> (+22.91%) ⬆️
ogre2/src/Ogre2LidarVisual.cc 81.55% <20.00%> (+0.87%) ⬆️
ogre2/src/Ogre2DynamicRenderable.cc 73.42% <50.00%> (-1.58%) ⬇️
ogre2/src/Ogre2SelectionBuffer.cc 93.17% <83.87%> (+93.17%) ⬆️
ogre2/src/Ogre2RenderTarget.cc 85.38% <0.00%> (+0.92%) ⬆️
src/base/BaseScene.cc 72.54% <0.00%> (+1.84%) ⬆️
ogre2/src/Ogre2Scene.cc 76.80% <0.00%> (+2.46%) ⬆️
... and 7 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 8143932...2eb00dd. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Dec 2, 2021

works for me. I made some changes in the iche033/6/marker_point_color branch for Metal. I don't have a working mac setup to test it though.

Using the normal buffer is fine. I know that in ogre 1.x, you can create vertex element of type VES_DIFFUSE to store colors per vertex (e.g. in place of VES_NORMAL if operationType is point list) but I couldn't find examples that does it in ogre 2.x so not sure how well it's working. Otherwise, that would probably be a more semantically correct way to pass colors to shaders.

@chapulina
Copy link
Contributor Author

I made some changes in the iche033/6/marker_point_color branch for Metal.

Thanks! Merged in.

chapulina and others added 2 commits December 6, 2021 15:24
* fix selection buffer crash due to resize and incorrect selections

Signed-off-by: Ian Chen <[email protected]>

* test updating full selection buffer texture

Signed-off-by: Ian Chen <[email protected]>

* reenable visual at test

Signed-off-by: Ian Chen <[email protected]>

* fix codecheck

Signed-off-by: Ian Chen <[email protected]>

* testing rgb no depth, full buffer

Signed-off-by: Ian Chen <[email protected]>

* use 1x1 buffer, still no depth data

Signed-off-by: Ian Chen <[email protected]>

* prnt scaling factor

Signed-off-by: Ian Chen <[email protected]>

* disable device ratio

Signed-off-by: Ian Chen <[email protected]>

* reenable depth and utils test

Signed-off-by: Ian Chen <[email protected]>

* disable utils test, add visual at test after resize

Signed-off-by: Ian Chen <[email protected]>

* test texelfetch

Signed-off-by: Ian Chen <[email protected]>

* back to full buffer

Signed-off-by: Ian Chen <[email protected]>

* print ogre log

Signed-off-by: Ian Chen <[email protected]>

* codecheck

Signed-off-by: Ian Chen <[email protected]>

* fixing selection buffer mat script

Signed-off-by: Ian Chen <[email protected]>

* try 1x1 buffer again

Signed-off-by: Ian Chen <[email protected]>

* revert some test changes

Signed-off-by: Ian Chen <[email protected]>

* uncomment tests

Signed-off-by: Ian Chen <[email protected]>

* update scaling factor

Signed-off-by: Ian Chen <[email protected]>

* fix removing selection mat

Signed-off-by: Ian Chen <[email protected]>

* update screenScalingFactor

Signed-off-by: Ian Chen <[email protected]>

* minor tweak

Signed-off-by: Ian Chen <[email protected]>

Co-authored-by: Louise Poubel <[email protected]>
@chapulina chapulina force-pushed the chapulina/6/marker_point_color branch from 6e52de8 to d88e99e Compare December 6, 2021 23:24
@chapulina chapulina changed the base branch from ign-rendering6 to main December 6, 2021 23:24
@chapulina chapulina added 🌱 garden Ignition Garden and removed 🏯 fortress Ignition Fortress labels Dec 6, 2021
@chapulina
Copy link
Contributor Author

FYI, I rebased this PR to Garden (main), because downstream PRs have breaking changes that need to target Garden. If there's a need to backport this to Fortress later I can open a separate PR.

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 to me.

srmainwaring pushed a commit to srmainwaring/gz-rendering that referenced this pull request Jan 26, 2022
srmainwaring pushed a commit to srmainwaring/gz-rendering that referenced this pull request Jan 26, 2022
iche033 added a commit that referenced this pull request Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants