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

Fix color of last / extra marker points #519

Merged
merged 1 commit into from
Dec 22, 2021

Conversation

chapulina
Copy link
Contributor

🦟 Bug fix

Summary

On #494 I had skipped filling the rest of the buffer for points because that was adding multiple points to the end of the cloud with the color blue. What I didn't notice is that I swapped the "last blue points" for "extra black point at the origin" 🙃

  • "last blue points" issue: the end of the vertex buffer is filled with points that have a Z normal (0,0,1 : blue) and coincide with the last point
  • "extra black point at the origin": if the vertex buffer isn't filled completely with points, it's filled with zeroes (origin position, black color)

The solution here is to first fill the buffer with blue normals, then override that with the color of the last point. I did this to match the implementation for other markers and reduce the amount of code changes, but some drawbacks to this approach are:

  • We have multiple overlapping points in one place, which could be a bit wasteful depending on the size of the buffer. Ideally it would be good to shrink the buffer, but I don't know if that's possible.
  • We're assigning the colors to the buffer twice, but we should be able to do it only once
  • I think other markers may be suffering from the same problem, but ending up with Z normals instead of blue colors. That's because GenerateNormals is not using the whole vertexBufferCapacity

Checklist

  • Signed all commits for DCO
  • Added tests - I don't know how to
  • 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.

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

@chapulina chapulina added the bug Something isn't working label Dec 21, 2021
@chapulina chapulina requested a review from iche033 as a code owner December 21, 2021 19:55
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Dec 21, 2021
@codecov
Copy link

codecov bot commented Dec 21, 2021

Codecov Report

Merging #519 (bb6f668) into main (dad0561) will decrease coverage by 0.00%.
The diff coverage is 40.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #519      +/-   ##
==========================================
- Coverage   54.06%   54.05%   -0.01%     
==========================================
  Files         203      203              
  Lines       20288    20287       -1     
==========================================
- Hits        10968    10966       -2     
- Misses       9320     9321       +1     
Impacted Files Coverage Δ
ogre2/src/Ogre2DynamicRenderable.cc 73.06% <40.00%> (-0.37%) ⬇️

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 dad0561...bb6f668. Read the comment docs.

@iche033
Copy link
Contributor

iche033 commented Dec 22, 2021

We have multiple overlapping points in one place, which could be a bit wasteful depending on the size of the buffer. Ideally it would be good to shrink the buffer, but I don't know if that's possible.

There is some logic to update the vertex buffer size so that it can dynamically increase / shrink if needed, though it tries not do it often as it requires destroying and recreating the buffers and ogre resources. For use case like lidar visualization in which the number of points are always fixed, I think one idea to improve our logic is to provide an API that allow users to specify the exact initial vertex buffer size so we don't need to duplicate points at the end of the vertex buffer.

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.

changes look good to me.

@chapulina chapulina merged commit 389e217 into main Dec 22, 2021
@chapulina chapulina deleted the chapulina/7/last_vertex_color branch December 22, 2021 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🌱 garden Ignition Garden
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants