From fa5d23c02c9097357363ebdab2f05a3ee9e12a9b Mon Sep 17 00:00:00 2001 From: Glenn Waldron Date: Tue, 22 Oct 2024 15:01:18 -0400 Subject: [PATCH] Review move sematics and convert the featuresource iterator to a stack object --- src/apps/rocky_demo/Demo_LineFeatures.h | 4 +- src/apps/rocky_demo/Demo_PolygonFeatures.h | 4 +- src/rocky/Feature.cpp | 25 +++++----- src/rocky/Feature.h | 49 +++++++++++++++----- src/rocky/GeoExtent.cpp | 12 ----- src/rocky/GeoExtent.h | 7 +-- src/rocky/GeoHeightfield.cpp | 16 ++++--- src/rocky/GeoHeightfield.h | 4 +- src/rocky/GeoImage.cpp | 8 +++- src/rocky/GeoImage.h | 4 +- src/rocky/Geocoder.cpp | 4 +- src/rocky/Heightfield.h | 16 +++---- src/rocky/Image.cpp | 53 +++++----------------- src/rocky/Image.h | 14 ++---- src/rocky/SRS.cpp | 20 -------- src/rocky/SRS.h | 12 ++--- src/rocky/vsg/FeatureView.cpp | 2 +- 17 files changed, 106 insertions(+), 148 deletions(-) diff --git a/src/apps/rocky_demo/Demo_LineFeatures.h b/src/apps/rocky_demo/Demo_LineFeatures.h index 93af8daf..97aaca38 100644 --- a/src/apps/rocky_demo/Demo_LineFeatures.h +++ b/src/apps/rocky_demo/Demo_LineFeatures.h @@ -45,9 +45,9 @@ auto Demo_LineFeatures = [](Application& app) FeatureView& feature_view = app.entities.emplace(entity); auto iter = data->fs->iterate(app.instance.io()); - while (iter->hasMore()) + while (iter.hasMore()) { - auto feature = iter->next(); + auto feature = iter.next(); if (feature.valid()) { // convert anything we find to lines: diff --git a/src/apps/rocky_demo/Demo_PolygonFeatures.h b/src/apps/rocky_demo/Demo_PolygonFeatures.h index f9841d35..3e058ab7 100644 --- a/src/apps/rocky_demo/Demo_PolygonFeatures.h +++ b/src/apps/rocky_demo/Demo_PolygonFeatures.h @@ -48,9 +48,9 @@ auto Demo_PolygonFeatures = [](Application& app) feature_view.features.reserve(data->fs->featureCount()); auto iter = data->fs->iterate(app.instance.io()); - while (iter->hasMore()) + while (iter.hasMore()) { - auto feature = iter->next(); + auto feature = iter.next(); if (feature.valid()) { feature.interpolation = GeodeticInterpolation::RhumbLine; diff --git a/src/rocky/Feature.cpp b/src/rocky/Feature.cpp index e78064c3..e0d6cfd4 100644 --- a/src/rocky/Feature.cpp +++ b/src/rocky/Feature.cpp @@ -478,7 +478,7 @@ OGRFeatureSource::~OGRFeatureSource() close(); } -std::shared_ptr +FeatureSource::iterator OGRFeatureSource::iterate(IOOptions& io) { OGRDataSourceH dsHandle = nullptr; @@ -507,14 +507,14 @@ OGRFeatureSource::iterate(IOOptions& io) } } + auto i = new iterator_impl(); + if (layerHandle) { - auto i = std::make_shared(); i->_source = this; i->_dsHandle = dsHandle; i->_layerHandle = layerHandle; i->init(); - return i; } else { @@ -523,18 +523,16 @@ OGRFeatureSource::iterate(IOOptions& io) OGRReleaseDataSource(dsHandle); } } - return { }; + return iterator(i); } -OGRFeatureSource::iterator::iterator() -{ -} - void -OGRFeatureSource::iterator::init() +OGRFeatureSource::iterator_impl::init() { + _resultSetEndReached = false; + if (_dsHandle) { std::string from = OGR_FD_GetName(OGR_L_GetLayerDefn(_layerHandle)); @@ -564,9 +562,8 @@ OGRFeatureSource::iterator::init() readChunk(); } -OGRFeatureSource::iterator::~iterator() +OGRFeatureSource::iterator_impl::~iterator_impl() { - if (_nextHandleToQueue) { OGR_F_Destroy(_nextHandleToQueue); @@ -574,7 +571,7 @@ OGRFeatureSource::iterator::~iterator() } void -OGRFeatureSource::iterator::readChunk() +OGRFeatureSource::iterator_impl::readChunk() { if (!_resultSetHandle) return; @@ -617,13 +614,13 @@ OGRFeatureSource::iterator::readChunk() } bool -OGRFeatureSource::iterator::hasMore() const +OGRFeatureSource::iterator_impl::hasMore() const { return _resultSetHandle && !_queue.empty(); }; const Feature& -OGRFeatureSource::iterator::next() +OGRFeatureSource::iterator_impl::next() { if (!hasMore()) return _lastFeatureReturned; diff --git a/src/rocky/Feature.h b/src/rocky/Feature.h index dc69b86a..03d8275e 100644 --- a/src/rocky/Feature.h +++ b/src/rocky/Feature.h @@ -63,6 +63,11 @@ namespace ROCKY_NAMESPACE //! Copy constructor Geometry(const Geometry& rhs) = default; + Geometry& operator=(const Geometry& rhs) = default; + + //! Move constructor + Geometry(Geometry&& rhs) noexcept = default; + Geometry& operator=(Geometry&& rhs) noexcept = default; //! Contruct a typed geometry by copying points from an existing range //! @param type Geometry type @@ -209,7 +214,14 @@ namespace ROCKY_NAMESPACE //! Construct an empty feature object. Feature() = default; + + //! Copy constructor Feature(const Feature& rhs) = default; + Feature& operator =(const Feature& rhs) = default; + + //! Move constructor + Feature(Feature&& rhs) noexcept = default; + Feature& operator =(Feature&& rhs) noexcept = default; //! Whether the feature is valid bool valid() const { @@ -230,7 +242,9 @@ namespace ROCKY_NAMESPACE void dirtyExtent(); }; - + /** + * Extent, SRS, and possibly the tiling profile of a feautre source + */ struct FeatureProfile { GeoExtent extent; @@ -246,15 +260,26 @@ namespace ROCKY_NAMESPACE class iterator { public: - virtual bool hasMore() const = 0; - virtual const Feature& next() = 0; + struct implementation { + virtual bool hasMore() const = 0; + virtual const Feature& next() = 0; + }; + + public: + bool hasMore() const { return _impl->hasMore(); } + const Feature& next() { return _impl->next(); } + + iterator(implementation* impl) : _impl(impl) { } + + private: + std::unique_ptr _impl; }; //! Number of features, or -1 if the count isn't available virtual int featureCount() const = 0; //! Creates a feature iterator - virtual std::shared_ptr iterate(IOOptions& io) = 0; + virtual iterator iterate(IOOptions& io) = 0; }; @@ -284,7 +309,7 @@ namespace ROCKY_NAMESPACE void close(); //! Create an interator to read features from the source - std::shared_ptr iterate(IOOptions& io) override; + FeatureSource::iterator iterate(IOOptions& io) override; //! Number of features, or -1 if the count isn't available int featureCount() const override; @@ -302,28 +327,28 @@ namespace ROCKY_NAMESPACE FeatureProfile _featureProfile; std::string _source; - class ROCKY_EXPORT iterator : public FeatureSource::iterator + class ROCKY_EXPORT iterator_impl : public FeatureSource::iterator::implementation { public: - iterator(); - void init(); - ~iterator(); + ~iterator_impl(); bool hasMore() const override; const Feature& next() override; private: std::queue _queue; Feature _lastFeatureReturned; - void readChunk(); - friend class OGRFeatureSource; OGRFeatureSource* _source = nullptr; void* _dsHandle = nullptr; void* _layerHandle = nullptr; void* _resultSetHandle = nullptr; void* _spatialFilterHandle = nullptr; void* _nextHandleToQueue = nullptr; - bool _resultSetEndReached = false; + bool _resultSetEndReached = true; const std::size_t _chunkSize = 500; Feature::ID _idGenerator = 1; + + void init(); + void readChunk(); + friend class OGRFeatureSource; }; }; diff --git a/src/rocky/GeoExtent.cpp b/src/rocky/GeoExtent.cpp index 9c8c3e43..512505c8 100644 --- a/src/rocky/GeoExtent.cpp +++ b/src/rocky/GeoExtent.cpp @@ -29,18 +29,6 @@ GeoExtent::GeoExtent() : //NOP - invalid } -GeoExtent& -GeoExtent::operator =(GeoExtent&& rhs) -{ - _srs = rhs._srs; - _west = rhs._west; - _width = rhs._width; - _south = rhs._south; - _height = rhs._height; - rhs._srs = { }; - return *this; -} - GeoExtent::GeoExtent(const SRS& srs) : _srs(srs), _west(0.0), diff --git a/src/rocky/GeoExtent.h b/src/rocky/GeoExtent.h index f5a2739f..ad766a30 100644 --- a/src/rocky/GeoExtent.h +++ b/src/rocky/GeoExtent.h @@ -22,8 +22,8 @@ namespace ROCKY_NAMESPACE GeoExtent(); GeoExtent(const GeoExtent& rhs) = default; GeoExtent& operator=(const GeoExtent&) = default; - GeoExtent(GeoExtent&& rhs) { *this = rhs; } - GeoExtent& operator=(GeoExtent&&); + GeoExtent(GeoExtent&& rhs) noexcept = default; + GeoExtent& operator=(GeoExtent&&) noexcept = default; /** Contructs a valid extent */ GeoExtent( @@ -39,9 +39,6 @@ namespace ROCKY_NAMESPACE const SRS& srs, const Box& bounds); - /** dtor */ - virtual ~GeoExtent() { } - //! Set from the SW and NE corners. void set(double west, double south, double east, double north); diff --git a/src/rocky/GeoHeightfield.cpp b/src/rocky/GeoHeightfield.cpp index 2c6f7f2a..d6887914 100644 --- a/src/rocky/GeoHeightfield.cpp +++ b/src/rocky/GeoHeightfield.cpp @@ -18,17 +18,21 @@ GeoHeightfield::GeoHeightfield() : } GeoHeightfield& -GeoHeightfield::operator=(GeoHeightfield&& rhs) +GeoHeightfield::operator=(GeoHeightfield&& rhs) noexcept { - *this = (const GeoHeightfield&)rhs; + if (this != &rhs) + { + _hf = std::move(rhs._hf); + _extent = std::move(rhs._extent); + _minHeight = rhs._minHeight; + _maxHeight = rhs._maxHeight; + _resolution = rhs._resolution; + } rhs._extent = GeoExtent::INVALID; return *this; } -GeoHeightfield::GeoHeightfield( - shared_ptr heightField, - const GeoExtent& extent) : - +GeoHeightfield::GeoHeightfield(shared_ptr heightField, const GeoExtent& extent) : _hf(heightField), _extent(extent), _minHeight(FLT_MAX), diff --git a/src/rocky/GeoHeightfield.h b/src/rocky/GeoHeightfield.h index 7e9b1c65..66c36477 100644 --- a/src/rocky/GeoHeightfield.h +++ b/src/rocky/GeoHeightfield.h @@ -22,8 +22,8 @@ namespace ROCKY_NAMESPACE GeoHeightfield(); GeoHeightfield(const GeoHeightfield&) = default; GeoHeightfield& operator=(const GeoHeightfield&) = default; - GeoHeightfield(GeoHeightfield&& rhs) { *this = rhs; } - GeoHeightfield& operator=(GeoHeightfield&& rhs); + GeoHeightfield(GeoHeightfield&& rhs) noexcept { *this = rhs; } + GeoHeightfield& operator=(GeoHeightfield&& rhs) noexcept; //! Constructs a new georeferenced heightfield. GeoHeightfield( diff --git a/src/rocky/GeoImage.cpp b/src/rocky/GeoImage.cpp index 6a311910..154c3d92 100644 --- a/src/rocky/GeoImage.cpp +++ b/src/rocky/GeoImage.cpp @@ -545,9 +545,13 @@ GeoImage::GeoImage() : } GeoImage& -GeoImage::operator=(GeoImage&& rhs) +GeoImage::operator=(GeoImage&& rhs) noexcept { - *this = (const GeoImage&)rhs; + if (this != &rhs) + { + _image = std::move(rhs._image); + _extent = std::move(rhs._extent); + } rhs._image = nullptr; rhs._extent = GeoExtent::INVALID; return *this; diff --git a/src/rocky/GeoImage.h b/src/rocky/GeoImage.h index dcc6093c..bbe8f32a 100644 --- a/src/rocky/GeoImage.h +++ b/src/rocky/GeoImage.h @@ -25,8 +25,8 @@ namespace ROCKY_NAMESPACE GeoImage(); GeoImage(const GeoImage&) = default; GeoImage& operator=(const GeoImage&) = default; - GeoImage(const GeoImage&& rhs) { *this = rhs; } - GeoImage& operator=(GeoImage&&); + GeoImage(const GeoImage&& rhs) noexcept { *this = rhs; } + GeoImage& operator=(GeoImage&&) noexcept; //! Constructs a new goereferenced image. GeoImage(shared_ptr image, const GeoExtent& extent); diff --git a/src/rocky/Geocoder.cpp b/src/rocky/Geocoder.cpp index ff3fee68..e8eef89e 100644 --- a/src/rocky/Geocoder.cpp +++ b/src/rocky/Geocoder.cpp @@ -29,9 +29,9 @@ Geocoder::geocode(const std::string& location, IOOptions& io) const fs->externalLayerHandle = layerHandle; fs->externalSRS = SRS::WGS84; auto iter = fs->iterate(io); - while (iter->hasMore()) + while (iter.hasMore()) { - result.emplace_back(iter->next()); + result.emplace_back(iter.next()); } OGRGeocodeFreeResult(layerHandle); } diff --git a/src/rocky/Heightfield.h b/src/rocky/Heightfield.h index 92952d0b..e1558ca6 100644 --- a/src/rocky/Heightfield.h +++ b/src/rocky/Heightfield.h @@ -37,11 +37,11 @@ namespace ROCKY_NAMESPACE //! Visits each height in the field with a user-provided function //! that takes "float" or "float&" as an argument. - template - void forEachHeight(FUNC func); + template + inline void forEachHeight(CALLABLE&& func); - template - void forEachHeight(FUNC func) const; + template + inline void forEachHeight(CALLABLE&& func) const; //! Interpolated height at a normalized (u,v) location float heightAtUV( @@ -70,16 +70,16 @@ namespace ROCKY_NAMESPACE return data(c, r); } - template - void Heightfield::forEachHeight(FUNC func) + template + void Heightfield::forEachHeight(CALLABLE&& func) { float* ptr = data(); for (auto i = 0u; i < sizeInPixels(); ++i, ++ptr) func(*ptr); } - template - void Heightfield::forEachHeight(FUNC func) const + template + void Heightfield::forEachHeight(CALLABLE&& func) const { const float* ptr = data(); for (auto i = 0u; i < sizeInPixels(); ++i, ++ptr) diff --git a/src/rocky/Image.cpp b/src/rocky/Image.cpp index 8fd96a43..cd47974b 100644 --- a/src/rocky/Image.cpp +++ b/src/rocky/Image.cpp @@ -73,12 +73,7 @@ Image::Layout Image::_layouts[7] = { &FLOAT::read, &FLOAT::write, 1, 8, R64_SFLOAT } }; -Image::Image( - PixelFormat format, - unsigned cols, - unsigned rows, - unsigned depth) : - +Image::Image(PixelFormat format, unsigned cols, unsigned rows, unsigned depth) : super(), _width(0), _height(0), _depth(0), _pixelFormat(R8G8B8A8_UNORM), @@ -94,13 +89,17 @@ Image::Image(const Image& rhs) : memcpy(_data, rhs._data, sizeInBytes()); } -Image::Image(Image&& rhs) +Image::Image(Image&& rhs) noexcept : + super(rhs) { - _width = rhs._width; - _height = rhs._height; - _depth = rhs._depth; - _pixelFormat = rhs._pixelFormat; - _data = rhs.releaseData(); + if (this != &rhs) + { + _width = rhs._width; + _height = rhs._height; + _depth = rhs._depth; + _pixelFormat = rhs._pixelFormat; + _data = rhs.releaseData(); + } } Image::~Image() @@ -172,36 +171,6 @@ Image::releaseData() return released; } -bool -Image::copyAsSubImage( - Image* dst, - unsigned dst_start_col, - unsigned dst_start_row) const -{ - if (!valid() || !dst || !dst->valid() || - dst_start_col + width() > dst->width() || - dst_start_row + height() > dst->height() || - depth() != dst->depth()) - { - return false; - } - - Pixel pixel; - for (unsigned r = 0; r < depth(); ++r) - { - for (unsigned src_t = 0, dst_t = dst_start_row; src_t < height(); src_t++, dst_t++) - { - for (unsigned src_s = 0, dst_s = dst_start_col; src_s < width(); src_s++, dst_s++) - { - read(pixel, src_s, src_t, r); - dst->write(pixel, dst_s, dst_t, r); - } - } - } - - return true; -} - void Image::flipVerticalInPlace() { diff --git a/src/rocky/Image.h b/src/rocky/Image.h index 16ef6f84..cff64760 100644 --- a/src/rocky/Image.h +++ b/src/rocky/Image.h @@ -75,7 +75,7 @@ namespace ROCKY_NAMESPACE Image(const Image& rhs); //! Move constructor - Image(Image&& rhs); + Image(Image&& rhs) noexcept; //! Destruct and release the data unless it's not owned virtual ~Image(); @@ -169,12 +169,6 @@ namespace ROCKY_NAMESPACE //! Inverts the pixels in the T dimension void flipVerticalInPlace(); - //! Copy this entire image to a sub-location in another image - bool copyAsSubImage( - Image* destination, - unsigned destX, - unsigned destY) const; - //! Nmmber of components in this image's pixel format inline unsigned numComponents() const; @@ -192,9 +186,9 @@ namespace ROCKY_NAMESPACE inline void forEachPixel(CALLABLE&& func); private: - const Image* _image; - unsigned _r, _s, _t; - double _u, _v; + const Image* _image = nullptr; + unsigned _r = 0, _s = 0, _t = 0; + double _u = 0.0, _v = 0.0; }; iterator get_iterator() const { diff --git a/src/rocky/SRS.cpp b/src/rocky/SRS.cpp index 792485f7..a1eea7c4 100644 --- a/src/rocky/SRS.cpp +++ b/src/rocky/SRS.cpp @@ -808,26 +808,6 @@ SRS::string() const return ""; } -SRSOperation::SRSOperation() -{ - // nop -} - -SRSOperation& -SRSOperation::operator=(SRSOperation&& rhs) -{ - _from = rhs._from; - _to = rhs._to; - rhs._from = { }; - rhs._to = { }; - return *this; -} - -SRSOperation::~SRSOperation() -{ - //nop -} - bool SRSOperation::valid() const { diff --git a/src/rocky/SRS.h b/src/rocky/SRS.h index d412978d..8c66d07b 100644 --- a/src/rocky/SRS.h +++ b/src/rocky/SRS.h @@ -142,7 +142,9 @@ namespace ROCKY_NAMESPACE // copy/move operations SRS(const SRS& rhs) = default; SRS& operator=(const SRS& rhs) = default; - // no move operators since we use global instances + SRS(SRS&&) = default; + SRS& operator =(SRS&& rhs) = default; + ~SRS(); //! Internal SRS representation (for debugging) @@ -173,8 +175,6 @@ namespace ROCKY_NAMESPACE class ROCKY_EXPORT SRSOperation { public: - //! Construct an empty (invalid) operation - SRSOperation(); //! Whether this is a valid and legal operation bool valid() const; @@ -273,11 +273,11 @@ namespace ROCKY_NAMESPACE std::string string() const; // copy/move ops + SRSOperation() = default; SRSOperation(const SRSOperation& rhs) = default; SRSOperation& operator=(const SRSOperation&) = default; - SRSOperation(SRSOperation&& rhs) { *this = rhs; } - SRSOperation& operator=(SRSOperation&&); - ~SRSOperation(); + SRSOperation(SRSOperation&& rhs) noexcept = default; + SRSOperation& operator=(SRSOperation&&) noexcept = default; private: SRS _from; diff --git a/src/rocky/vsg/FeatureView.cpp b/src/rocky/vsg/FeatureView.cpp index 2cdecbb6..550f6381 100644 --- a/src/rocky/vsg/FeatureView.cpp +++ b/src/rocky/vsg/FeatureView.cpp @@ -104,7 +104,7 @@ namespace } output.push_back(input.back()); } - return std::move(output); + return output; } float get_max_segment_length(const std::vector& input)