Add Smooth scaling option to Image and Camera displays#1740
Open
abaeyens wants to merge 5 commits into
Open
Conversation
Adds a "Smooth scaling" BoolProperty (default off) to the Image and
Camera displays. When enabled, ROSImageTexture allocates a full mipmap
chain and the displays' materials sample it with TFO_TRILINEAR; when
disabled, the previous nearest-neighbour behaviour is preserved exactly.
Both displays previously sampled their texture with TFO_NONE and
allocated no mipmaps, so any minification (window smaller than image,
or non-1:1 zoom) fell back to nearest-neighbour and showed aliasing on
detailed content. The new option mirrors the "Smooth scaling" checkbox
in rqt_image_view (matching label and default-off). The underlying
implementation differs: GPU-side trilinear on a hardware-generated mip
chain here, CPU-side Qt::SmoothTransformation in rqt. This
implementation also covers the Camera display's frustum quads.
Implementation notes:
- ensureTexture() keeps a single Ogre::TexturePtr across resizes and
smooth-scaling toggles. Required because Ogre::TextureUnitState
caches the resolved TexturePtr after the first setTextureName
lookup; recreating the texture under the same name would leave that
cache dangling.
- Reconfigure uses freeInternalResources() + createInternalResources(),
not unload() + load(). unload() is a no-op for a manually-created
texture whose LoadingState is UNLOADED, and the subsequent
createInternalResources() short-circuits on
mInternalResourcesCreated == true, silently dropping the mip-chain
rebuild.
- MIP_UNLIMITED, not MIP_DEFAULT, is the right sentinel for
setNumMipmaps(). setNumMipmaps() takes uint32 and does no
translation, so MIP_DEFAULT (-1) would become 0xFFFFFFFF before
backend clamping. Same end state on GL3+ in practice, but
semantically wrong and fragile against future RenderSystems.
- update() now uploads incoming frames via in-place blitFromMemory
rather than the previous unload() + loadImage() (which allocated an
Ogre::Image + MemoryDataStream per frame and re-created the GL
texture). This is required for texture-pointer stability above, and
removes a per-frame heap + GL allocation on the smooth-scaling-off
path too.
- setSmoothScaling() re-arms new_image_ when a frame is held so that
the next update() re-uploads the live frame; without this, latched
topics and paused bags would lose the live image on toggle and
display the no_image.png placeholder until the next publish.
- ROSImageTextureIface::setSmoothScaling has a {} default
implementation rather than =0 so third-party plugins injecting their
own texture via ImageDisplay's public constructor keep building and
silently ignore the setting until they opt in.
Tests:
- default_construction_does_not_allocate_a_mipmap_chain
- enabling_smooth_scaling_allocates_a_mipmap_chain (asserts exact mip
count = floor(log2(max(w, h))) to catch sentinel-vs-clamp drift)
- toggling_smooth_scaling_preserves_the_texture_pointer
- toggling_smooth_scaling_with_a_held_image_re_uploads_it (regression
for the placeholder-clobber-on-toggle bug noted above)
- update_with_smooth_scaling_writes_new_image_to_the_texture
(verifies mip level 1 has content, not just allocation)
- destructor_removes_the_texture_from_the_manager
- initialize_propagates_smooth_scaling_to_texture (on ImageDisplay)
Signed-off-by: Arne Baeyens <mail@arnebaeyens.com>
ahcorde
requested changes
May 21, 2026
| void clear(); | ||
|
|
||
| Ogre::MaterialPtr createMaterial(std::string name) const; | ||
| Ogre::MaterialPtr createMaterial(std::string name); |
| // display should fall back to TFO_NONE. The default implementation is a | ||
| // no-op so that out-of-tree implementors of this interface keep building | ||
| // and silently ignore the setting until they opt in. | ||
| virtual void setSmoothScaling(bool /*enabled*/) {} |
Contributor
There was a problem hiding this comment.
breaking ABI here, not a problem on rolling but this is not backportable
Contributor
Author
There was a problem hiding this comment.
True. TBH I'd appreciate being able to use this half-feature-half-bugfix in Jazzy and especially Lyrical (which, I assume, requires a backport given it already got released).
With the change below, we can keep ABI, though it's not as neat (because it moves the setSmoothScaling method out of the interface parent class and also removes a test). I suggest we go with the current approach for rolling, and to backport apply this additional diff. What's your view on this?
diff --git a/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture.hpp b/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture.hpp
index cab30acb..f38935b1 100644
--- a/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture.hpp
+++ b/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture.hpp
@@ -122,8 +122,17 @@ public:
RVIZ_DEFAULT_PLUGINS_PUBLIC
void setMedianFrames(unsigned median_frames) override;
+ // When enabled, the texture is built with a full mipmap chain (driver
+ // generates mips on each upload via TU_AUTOMIPMAP). The display's material
+ // should pair this with a trilinear or anisotropic filter to actually
+ // sample the chain; when disabled, the texture has no mip chain and the
+ // display should fall back to TFO_NONE. The default implementation is a
+ // no-op so that out-of-tree implementors of this interface keep building
+ // and silently ignore the setting until they opt in.
+ // Note: put here instead of in ROSImageTextureIface
+ // to avoid breaking ABI in this backport.
RVIZ_DEFAULT_PLUGINS_PUBLIC
- void setSmoothScaling(bool enabled) override;
+ void setSmoothScaling(bool enabled);
private:
// Ensures the underlying Ogre texture matches the requested dimensions,
diff --git a/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture_iface.hpp b/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture_iface.hpp
index 5d348563..63963d4d 100644
--- a/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture_iface.hpp
+++ b/rviz_default_plugins/include/rviz_default_plugins/displays/image/ros_image_texture_iface.hpp
@@ -63,15 +63,6 @@ public:
virtual void setNormalizeFloatImage(bool normalize) = 0;
virtual void setNormalizeFloatImage(bool normalize, double min, double max) = 0;
virtual void setMedianFrames(unsigned median_frames) = 0;
-
- // When enabled, the texture is built with a full mipmap chain (driver
- // generates mips on each upload via TU_AUTOMIPMAP). The display's material
- // should pair this with a trilinear or anisotropic filter to actually
- // sample the chain; when disabled, the texture has no mip chain and the
- // display should fall back to TFO_NONE. The default implementation is a
- // no-op so that out-of-tree implementors of this interface keep building
- // and silently ignore the setting until they opt in.
- virtual void setSmoothScaling(bool /*enabled*/) {}
};
} // namespace displays
diff --git a/rviz_default_plugins/src/rviz_default_plugins/displays/image/image_display.cpp b/rviz_default_plugins/src/rviz_default_plugins/displays/image/image_display.cpp
index bd3f5c5f..1eb9af93 100644
--- a/rviz_default_plugins/src/rviz_default_plugins/displays/image/image_display.cpp
+++ b/rviz_default_plugins/src/rviz_default_plugins/displays/image/image_display.cpp
@@ -346,7 +346,9 @@ void ImageDisplay::updateNormalizeOptions()
void ImageDisplay::updateSmoothScaling()
{
- texture_->setSmoothScaling(smooth_scaling_property_->getBool());
+ if (auto * concrete = dynamic_cast<ROSImageTexture *>(texture_.get())) {
+ concrete->setSmoothScaling(smooth_scaling_property_->getBool());
+ }
applySmoothScalingToMaterial(material_);
}
diff --git a/rviz_default_plugins/test/rviz_default_plugins/displays/image/image_display_test.cpp b/rviz_default_plugins/test/rviz_default_plugins/displays/image/image_display_test.cpp
index d3b7177f..e7d0778b 100644
--- a/rviz_default_plugins/test/rviz_default_plugins/displays/image/image_display_test.cpp
+++ b/rviz_default_plugins/test/rviz_default_plugins/displays/image/image_display_test.cpp
@@ -138,17 +138,6 @@ TEST_F(ImageDisplayTestFixture, update_calls_texture_update) {
imageDisplay.update(zero, zero);
}
-TEST_F(ImageDisplayTestFixture, initialize_propagates_smooth_scaling_to_texture) {
- auto panelDockWidget = new rviz_common::PanelDockWidget("panelDockWidget");
- EXPECT_CALL(*window_manager_, addPane(_, _, _, _)).WillOnce(Return(panelDockWidget));
- EXPECT_CALL(*context_, getFixedFrame()).WillOnce(Return(""));
-
- EXPECT_CALL(*texture_, setSmoothScaling(false)).Times(AtLeast(1));
-
- ImageDisplay imageDisplay(std::move(texture_));
- imageDisplay.initialize(context_.get());
-}
-
int main(int argc, char ** argv)
{
QApplication app(argc, argv);
diff --git a/rviz_default_plugins/test/rviz_default_plugins/displays/image/mock_ros_image_texture.hpp b/rviz_default_plugins/test/rviz_default_plugins/displays/image/mock_ros_image_texture.hpp
index b08cd590..f277dd38 100644
--- a/rviz_default_plugins/test/rviz_default_plugins/displays/image/mock_ros_image_texture.hpp
+++ b/rviz_default_plugins/test/rviz_default_plugins/displays/image/mock_ros_image_texture.hpp
@@ -55,7 +55,6 @@ public:
MOCK_METHOD1(setNormalizeFloatImage, void(bool normalize));
MOCK_METHOD3(setNormalizeFloatImage, void(bool normalize, double min, double max));
MOCK_METHOD1(setMedianFrames, void(unsigned median_frames));
- MOCK_METHOD1(setSmoothScaling, void(bool enabled));
};
#endif // RVIZ_DEFAULT_PLUGINS__DISPLAYS__IMAGE__MOCK_ROS_IMAGE_TEXTURE_HPP_…mage-smooth-scaling Conflicts: rviz_default_plugins/src/rviz_default_plugins/displays/image/image_display.cpp
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Current situation
Image and Camera displays render images with nearest-neighbour downscaling, resulting in strong aliasing. Here's how an RGB zone plate looks like in RViz:
The "eyes" on the left and the right, as well as the five along the top and the bottom, are purely downscaling artifacts resulting from the downscaling algorithm RViz currently uses.
Situation with these changes
Here's what the same input image looks like when rendered with the approximate area-based weighting approach introduced by this PR:
Not perfect (some secondary circles still pop up), yet much better. Executing a quality downscale is a delicate balance between suppressing anti-aliasing effects, maintaining detail as well as being fast. Also, keep in mind that an RGB zone plate is designed to greatly exacerbate lack of proper anti-aliasing - the effect is less in the usual robotics camera picture. (And, in any case, much better than the current implementation.)
What it looks like in the Displays panel:
Explanation
Adds a "Smooth scaling" BoolProperty (default off) to the Image and Camera displays. When enabled,
ROSImageTextureallocates a full mipmap chain and the displays' materials sample it withTFO_TRILINEAR; when disabled, the previous nearest-neighbour behaviour is preserved exactly.Both displays previously sampled their texture with
TFO_NONEand allocated no mipmaps, so any minification (window smaller than image, or non-1:1 zoom) fell back to nearest-neighbour and showed aliasing on detailed content. The new option mirrors the "Smooth scaling" checkbox in rqt_image_view (matching label and default-off). The underlying implementation differs: GPU-side trilinear on a hardware-generated mip chain here, CPU-sideQt::SmoothTransformationin rqt. In practice, both these latter two algorithms deliver results that are close to OpenCV'sINTER_AREA. This implementation also covers the Camera display's frustum quads.Please see the commit message for more implementation details.
Is this user-facing behavior change?
Yes, please see above. The feature still defaults to disabled, though.
Did you use Generative AI?
Claude Code with Opus 4.7
I think this code is reasonably decent, though I'm not very familiar with this code base. If it's garbage, please let me know.
Additional Information
Whether by this or another PR, I'd appreciate that RViz gets this feature such that fine detail like overlay text is rendered nicely and reasonably correctly.