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 map display #425

Merged
merged 4 commits into from Jul 30, 2019

Conversation

@Martin-Idel
Copy link

Martin-Idel commented Jul 6, 2019

Fixes #424
Fixes #401

This was a bug introduced during migration:
The Ogre API allows textures of size at most unsigned short number of bytes height or width. However, the current implementation only creates textures of at most 8 bit height or width. Since the number of swatches was capped at 8 in the original RViz and this limit has been kept, this means that maps of moderate size (e.g. 800 * 800 pixels) crashed.

This PR fixes this bug by creating textures of 16 bit size.

I also improved the status quo:

  • If a map is too large, a message will be logged and the map will not be drawn.
  • I also added a comment on why the maximal number of swatches is capped at 8: One swatch can hold up to 4 GB of data, so needing more than 8 swatches will either need exceptional amounts of memory or maps which are very long and thin (i.e. hundreds of thousands of pixels of length).
  • If these turn out to be use cases, we can easily address them later.
@rotu

This comment has been minimized.

Copy link

rotu commented Jul 11, 2019

It would be nice if the logging messages were made more self-explanatory:

[INFO] [rviz2]: Stereo is NOT SUPPORTED
[INFO] [rviz2]: OpenGl version: 3 (GLSL 1.3)
[INFO] [rviz2]: Stereo is NOT SUPPORTED
[INFO] [rviz2]: Creating 1 swatches_

The user doesn't know what a swatch is. Maybe the message should be something like "Preparing to render a 400x400 map as 1 swatch"

@Martin-Idel

This comment has been minimized.

Copy link
Author

Martin-Idel commented Jul 12, 2019

@rotu Makes sense, I added it using your suggestion.

@rotu

This comment has been minimized.

Copy link

rotu commented Jul 26, 2019

This PR is working great for me. Why's it taking so long to get merged in?

Copy link
Member

jacobperron left a comment

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
@jacobperron

This comment has been minimized.

Copy link
Member

jacobperron commented Jul 29, 2019

I believe CI is failing because we need a rebase to get e37643f

Martin-Idel added 4 commits Jul 6, 2019
The test fails for the current implementation as the map is too big.
This should not be the case though.
The API of loadRawData takes an Ogre ushort which is
an unsigned short which is guaranteed to be 16 bit.
Hence we should static_cast to a uint16_t instead of a uint8_t.

This will result in the swatch being creatable for much larger
textures
- Explain rationale behind maximum swatch split
- Improve const correctness
- Log better error if the map cannot be displayed
- Move method updateDrawnUnder() to place where it
  can actually be executed
@Martin-Idel Martin-Idel force-pushed the Martin-Idel:bugfix/map_display branch from e8834d8 to f610629 Jul 30, 2019
@Martin-Idel

This comment has been minimized.

Copy link
Author

Martin-Idel commented Jul 30, 2019

@jacobperron I think you're right. I did the rebase and now it builds and tests cleanly on my machine.

I also took the liberty to retrigger CI (EDIT: new try by replacing --packages-test with --packages-select:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

It might be worth considering this also for a backport to dashing.

@jacobperron jacobperron merged commit d52e5c1 into ros2:ros2 Jul 30, 2019
jacobperron added a commit that referenced this pull request Jul 30, 2019
* Improve test for large maps

The test fails for the current implementation as the map is too big.
This should not be the case though.

* Fix swatches to store more data

The API of loadRawData takes an Ogre ushort which is
an unsigned short which is guaranteed to be 16 bit.
Hence we should static_cast to a uint16_t instead of a uint8_t.

This will result in the swatch being creatable for much larger
textures

* Minor refactoring of map display

- Explain rationale behind maximum swatch split
- Improve const correctness
- Log better error if the map cannot be displayed
- Move method updateDrawnUnder() to place where it
  can actually be executed

* Improve log messages
jacobperron added a commit that referenced this pull request Jul 31, 2019
* Improve test for large maps

The test fails for the current implementation as the map is too big.
This should not be the case though.

* Fix swatches to store more data

The API of loadRawData takes an Ogre ushort which is
an unsigned short which is guaranteed to be 16 bit.
Hence we should static_cast to a uint16_t instead of a uint8_t.

This will result in the swatch being creatable for much larger
textures

* Minor refactoring of map display

- Explain rationale behind maximum swatch split
- Improve const correctness
- Log better error if the map cannot be displayed
- Move method updateDrawnUnder() to place where it
  can actually be executed

* Improve log messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.