From 21ffb1938c207bdf569d3e28d6da0c96e46dfe60 Mon Sep 17 00:00:00 2001 From: Paul Meems Date: Fri, 6 Nov 2026 17:58:23 +0100 Subject: [PATCH 1/4] Fix full repository review findings Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .ai/memory/architecture/core-library.md | 6 + .ai/memory/build/dependencies.md | 2 + .ai/memory/lessons/lessons.md | 12 ++ .github/workflows/build.yml | 6 + Directory.Build.props | 2 +- Directory.Build.targets | 5 +- README.md | 3 + build/scripts/Test-BuildConfiguration.ps1 | 79 +++++++++ docs/ARCHITECTURE.md | 12 +- docs/BUILDING.md | 16 +- docs/TESTING.md | 11 ++ docs/gdal-integration.md | 13 ++ docs/gdal-modern-capabilities.md | 8 + .../GdiVectorLayerRenderer.cpp | 6 + .../mapwingis/gdal/GdalAlgorithmRunner.h | 12 +- .../include/mapwingis/gdal/GdalUtils.h | 2 + .../include/mapwingis/vector/VectorDataset.h | 11 ++ .../src/gdal/GdalAlgorithmRunner.cpp | 151 +++++++++++++++- src/MapWinGIS.Core/src/gdal/GdalUtils.cpp | 7 + .../src/vector/VectorDataset.cpp | 109 ++++++++++-- .../VectorLayerRendererTests.cpp | 36 ++++ .../GdalAlgorithmRunnerTests.cpp | 113 +++++++++++- .../GdalEnvironmentTests.cpp | 23 +++ tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp | 33 ++++ .../MapWinGIS.Core.Tests.vcxproj | 6 + tests/MapWinGIS.Core.Tests/TestData.cpp | 43 ++++- tests/MapWinGIS.Core.Tests/TestData.h | 11 +- .../MapWinGIS.Core.Tests/VectorLayerTests.cpp | 164 +++++++++++++++++- 28 files changed, 862 insertions(+), 40 deletions(-) create mode 100644 build/scripts/Test-BuildConfiguration.ps1 diff --git a/.ai/memory/architecture/core-library.md b/.ai/memory/architecture/core-library.md index 9598c33..c1bbf9e 100644 --- a/.ai/memory/architecture/core-library.md +++ b/.ai/memory/architecture/core-library.md @@ -26,6 +26,12 @@ GDAL/PROJ runtime data deployed beside the Core DLL. Do not rely on remain Core responsibilities; COM callbacks and host dispatch stay in ActiveX. +Algorithm outputs need different completion rules. Runner-owned persistent +outputs pass a checked close and reopen read-only before success. Anonymous +outputs retain their references for pipelines. Caller-supplied outputs remain +caller-owned, so the runner can check their flush but not their final close. +See `GdalAlgorithmRunner` and its output-failure and ownership tests. + Core render scenes are owned, backend-neutral snapshots. They are distinct from viewport state and are rebuilt when relevant map data changes. Source geometry edits made outside the map are not automatically observed; callers diff --git a/.ai/memory/build/dependencies.md b/.ai/memory/build/dependencies.md index 85fc024..25d5bcd 100644 --- a/.ai/memory/build/dependencies.md +++ b/.ai/memory/build/dependencies.md @@ -11,6 +11,8 @@ platform-specific tree because vcpkg can remove another triplet from a shared install directory. The build deploys GDAL and PROJ runtime data beside the output binaries. +Only the Core project's target deploys this shared data. Consumer projects +reference Core so parallel solution builds do not copy the same files at once. `GdalEnvironment` configures those paths; do not set `GDAL_DATA` or `PROJ_LIB` for the application. diff --git a/.ai/memory/lessons/lessons.md b/.ai/memory/lessons/lessons.md index 24193e5..1e48804 100644 --- a/.ai/memory/lessons/lessons.md +++ b/.ai/memory/lessons/lessons.md @@ -50,3 +50,15 @@ integration test still requires an interactive Windows desktop. **References:** [Testing guide](../../../docs/TESTING.md#activex-host-integration-checks) and [PR #26](https://github.com/MapWindow/MapWinGIS6/pull/26). + +## Shapefile geometry edits do not shrink stored bounds + +OGR's Shapefile driver expands bounds during `SetFeature` but does not shrink +them. Core runs `RECOMPUTE EXTENT ON` after each Shapefile geometry edit so +the stored extent remains correct after reopening. This costs an O(n) scan +per geometry edit. Attribute edits do not trigger it. A recomputation failure +throws after the geometry write succeeds, so callers must not assume rollback. + +See [VectorDataset.cpp](../../../src/MapWinGIS.Core/src/vector/VectorDataset.cpp) +and the shrinking and reopening cases in +[VectorLayerTests.cpp](../../../tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp). diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 954c11f..858a4f1 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -46,14 +46,20 @@ jobs: - name: Set up MSBuild uses: microsoft/setup-msbuild@v3 + - name: Check build configuration + shell: pwsh + run: ./build/scripts/Test-BuildConfiguration.ps1 + - name: Build shell: pwsh run: | + $features = if ('${{ matrix.configuration }}' -eq 'Release') { 'tests%3Bfull-drivers' } else { 'tests' } msbuild MapWinGIS6.slnx ` -restore ` -p:Configuration=${{ matrix.configuration }} ` -p:Platform=${{ matrix.platform }} ` -p:MwgWindowsSdk=$env:MWG_WINDOWS_SDK ` + "-p:MwgVcpkgFeatures=$features" ` -m -v:m - name: Test diff --git a/Directory.Build.props b/Directory.Build.props index a876f35..91a7b94 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -51,7 +51,7 @@ tests - --overlay-triplets=$(MwgRepoRoot)build\triplets --x-feature=$(MwgVcpkgFeatures.Replace(';',' --x-feature=')) + --overlay-triplets="$(MwgRepoRoot)build\triplets" --x-feature=$(MwgVcpkgFeatures.Replace(';',' --x-feature=')) diff --git a/Directory.Build.targets b/Directory.Build.targets index 6730c87..5d97501 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -6,7 +6,8 @@ vcpkg installs these under share/, and nothing copies them next to the binaries. GdalEnvironment looks for "gdal-data" and "proj" beside the - loaded module, so they are copied there. + loaded module, so the Core project copies them there once. All consumers + reference Core and share its output directory. --> @@ -15,7 +16,7 @@ + Condition="'$(MwgDeployRuntimeData)' == 'true' And '$(MSBuildProjectName)' == 'MapWinGIS.Core'"> diff --git a/README.md b/README.md index 1111b1d..a3ac6c7 100644 --- a/README.md +++ b/README.md @@ -80,6 +80,9 @@ Release configuration, build with `-p:Configuration=Release` and run For additional configurations, dependency features, and troubleshooting, see [docs/BUILDING.md](docs/BUILDING.md). +CI Release artifacts use the `full-drivers` feature. Their tests require +HDF5, NetCDF, KML, SpatiaLite, and PostgreSQL support. + ## Layout | Path | Contents | diff --git a/build/scripts/Test-BuildConfiguration.ps1 b/build/scripts/Test-BuildConfiguration.ps1 new file mode 100644 index 0000000..4d68e7b --- /dev/null +++ b/build/scripts/Test-BuildConfiguration.ps1 @@ -0,0 +1,79 @@ +<# +.SYNOPSIS + Tests dependency argument quoting and single-owner runtime deployment. +#> +[CmdletBinding()] +param( + [string] $MSBuild = 'msbuild' +) + +$ErrorActionPreference = 'Stop' +$repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '..\..')).Path +$temp = Join-Path ([System.IO.Path]::GetTempPath()) "MapWinGIS build checks $([guid]::NewGuid())" +try { + New-Item -ItemType Directory -Path "$temp\build\props" -Force | Out-Null + Copy-Item "$repoRoot\Directory.Build.props", "$repoRoot\Directory.Build.targets" $temp + Copy-Item "$repoRoot\build\props\MapWinGIS.Cpp.props" "$temp\build\props" + New-Item -ItemType Directory -Path "$temp\packages\share\gdal", "$temp\packages\share\proj" -Force | Out-Null + Set-Content "$temp\packages\share\gdal\test.csv" 'GDAL fixture' + Set-Content "$temp\packages\share\proj\proj.db" 'PROJ fixture' + Set-Content "$temp\capture.ps1" @' +$commandLine = [Environment]::GetCommandLineArgs() +$start = [array]::IndexOf($commandLine, $PSCommandPath) + 1 +$commandLine | Select-Object -Skip $start | Set-Content (Join-Path $PSScriptRoot 'arguments.txt') +'@ + + $project = @' + + + Debug + x64 + $(MSBuildThisFileDirectory)no-vcpkg + tests;full-drivers + + + + $(MSBuildThisFileDirectory)packages\ + $(MSBuildThisFileDirectory)output\ + + + + + + +'@ + Set-Content "$temp\MapWinGIS.Core.proj" $project + Set-Content "$temp\MapWinGIS.ActiveX.proj" $project + $powerShell = (Get-Process -Id $PID).Path + & $MSBuild "$temp\MapWinGIS.Core.proj" -nologo -v:q -t:CaptureArguments "-p:MwgTestPowerShell=$powerShell" + if ($LASTEXITCODE -ne 0) { throw 'Dependency argument capture failed.' } + $arguments = @(Get-Content "$temp\arguments.txt") + if ($arguments.Count -ne 3 -or + $arguments[0] -ne "--overlay-triplets=$temp\build\triplets" -or + $arguments[1] -ne '--x-feature=tests' -or + $arguments[2] -ne '--x-feature=full-drivers') { + throw "Dependency arguments were split or changed: $arguments" + } + + # A consumer must not copy, even when the shared output is empty. + & $MSBuild "$temp\MapWinGIS.ActiveX.proj" -nologo -v:q -t:MwgDeployGdalRuntimeData + if ($LASTEXITCODE -ne 0) { throw 'Consumer deployment check failed.' } + if (Test-Path "$temp\output") { throw 'A consumer copied shared runtime data.' } + + & $MSBuild "$temp\MapWinGIS.Core.proj" -nologo -v:q -t:MwgDeployGdalRuntimeData + if ($LASTEXITCODE -ne 0) { throw 'Core deployment failed.' } + foreach ($relative in @('gdal-data\test.csv', 'proj\proj.db')) { + if (-not (Test-Path "$temp\output\$relative")) { throw "Runtime data missing: $relative" } + } + + Set-Content "$temp\packages\share\proj\proj.db" 'Updated PROJ fixture' + & $MSBuild "$temp\MapWinGIS.ActiveX.proj" -nologo -v:q -t:MwgDeployGdalRuntimeData + if ($LASTEXITCODE -ne 0) { throw 'Consumer redeployment check failed.' } + if ((Get-Content "$temp\output\proj\proj.db") -ne 'PROJ fixture') { + throw 'A consumer overwrote shared runtime data.' + } + Write-Host 'Build configuration checks passed.' +} +finally { + if (Test-Path $temp) { Remove-Item -LiteralPath $temp -Recurse -Force } +} diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e9891de..ebafce2 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -138,6 +138,14 @@ state; hosts and renderers read from it. The map does not reproject; layers in different coordinate reference systems do not align until coordinate transformations are implemented. +Vector layer extent queries cache successful results. Read failures throw +without caching an empty result. If a driver cannot provide an extent and +reports no error, Core scans the geometries with read-error checks. +Attribute edits keep the cached extent. Shapefile geometry edits immediately +recompute stored bounds with an O(n) scan, so smaller bounds survive reopening. +If that recomputation fails, the edit throws after writing the geometry. +For other drivers, the next extent query scans once after a geometry edit. + ## Viewport `mwg::Viewport` (`mapwingis/map/Viewport.h`) is the view of map coordinates on @@ -387,7 +395,9 @@ reading feature geometries sequentially without WKT serialization and reparsing. Adding a prepared layer retains the existing scene. Other layer changes and explicit invalidation rebuild the scene for all visible layers. There is no tiling, paging, or clipping to the visible extent. GDI skips a -layer whose device coordinates exceed its range. The opt-in +layer whose device coordinates exceed its range. It clips drawing to the +requested target rectangle and the host's existing clip region, then restores +the device context. The opt-in rendering baseline (see [TESTING.md](TESTING.md#rendering-performance-baseline)) tracks scene preparation and backend frame costs. OpenGL only supports convex single-ring polygons and copies each frame through CPU memory. Its reusable CPU diff --git a/docs/BUILDING.md b/docs/BUILDING.md index c3d33bc..43631a2 100644 --- a/docs/BUILDING.md +++ b/docs/BUILDING.md @@ -131,6 +131,9 @@ Release with it again reinstalls the tree each time. 3. `..\vcpkg` next to the repository 4. the `VCPKG_ROOT` environment variable +The build quotes the overlay-triplet path when it invokes vcpkg, so checkout +paths can contain spaces. + ## Configurations and platforms | | | @@ -158,17 +161,24 @@ Manifest features: | `full-drivers` | Adds HDF5, NetCDF, KML, SpatiaLite and PostgreSQL to GDAL | Release packages. Much slower to build | ```powershell -msbuild MapWinGIS6.slnx -restore -p:Platform=x64 -p:Configuration=Release -p:MwgVcpkgFeatures="tests;full-drivers" +msbuild MapWinGIS6.slnx -restore -p:Platform=x64 -p:Configuration=Release "-p:MwgVcpkgFeatures=tests%3Bfull-drivers" ``` +`%3B` passes a literal semicolon through MSBuild's property parser. +CI uses this feature set for Release artifacts. Core tests built with +`full-drivers` require HDF5, NetCDF, LIBKML, PostgreSQL, and SpatiaLite support. +The SpatiaLite test creates a spatial database without an external server. + ### Runtime data vcpkg installs `gdal-data` and `proj.db` under `vcpkg_installed\\\share`. Each platform has its own install tree because vcpkg removes other triplets from a shared tree, which would break alternating Win32 and x64 builds. -A target in `Directory.Build.targets` copies them into the output folder as -`gdal-data\` and `proj\`, and `GdalEnvironment` points GDAL and PROJ at them. +The Core project's target in `Directory.Build.targets` copies them into the +output folder as `gdal-data\` and `proj\`. Other projects reference Core and +do not copy the shared data. This prevents competing writes during parallel +solution builds. `GdalEnvironment` points GDAL and PROJ at these folders. No `GDAL_DATA` or `PROJ_LIB` environment variable is needed or set. ### Binary cache diff --git a/docs/TESTING.md b/docs/TESTING.md index 522a1ba..233ca5c 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -24,6 +24,17 @@ bin\Release\x64\MapWinGIS.ActiveX.Tests.exe --gtest_output=xml:activex-testresul Also runnable from Visual Studio Test Explorer (GoogleTest adapter picks it up automatically). +Run the build configuration tests from a Visual Studio developer shell: + +```powershell +pwsh -File build\scripts\Test-BuildConfiguration.ps1 +``` + +These tests pass dependency arguments through MSBuild in a path with spaces. +They also make sure that only Core deploys GDAL and PROJ data to the shared +output folder. CI runs them before the solution build. Builds with +`full-drivers` also test the release driver set in `GdalEnvironment` tests. + ## ActiveX host integration checks The native `MapWinGIS.ActiveX.Tests` project covers COM activation, registered diff --git a/docs/gdal-integration.md b/docs/gdal-integration.md index 043107a..ee209cc 100644 --- a/docs/gdal-integration.md +++ b/docs/gdal-integration.md @@ -41,6 +41,15 @@ against GDAL's metadata and captures warnings and failures in `Errors()`. `Result()` returns a snapshot without transferring output datasets; `TakeOutputDataset()` transfers ownership of an output dataset. +For persistent outputs that the runner owns, `Run()` checks the final close +before reporting success. It reopens the output read-only for ownership +transfer. Reopening failures also fail the operation. Anonymous outputs, +including in-memory VRT datasets, remain open. If the caller supplies an +output through `SetDatasetArgument()`, the runner checks its flush but cannot +check its eventual close. The caller retains the original reference. +Output discovery follows the selected algorithm when command-line arguments +select a child of a group. + For common operations, use a typed options structure. This example converts an input raster to GeoTIFF and reports captured diagnostics on failure: @@ -89,6 +98,10 @@ algorithms promptly. Cancellation can leave partial output files. Callbacks must not start another GDAL operation or wait on a thread that needs the GDAL execution lock; callback exceptions are returned as operation failures. +The legacy `GdalUtils::GdalRasterWarp()` rejects `-multi` when a progress +callback is supplied. GDAL can call that callback from a worker thread with +this option. Calls without a progress callback can still use `-multi`. + ## ActiveX operations The ActiveX control exposes asynchronous generic GDAL execution through its diff --git a/docs/gdal-modern-capabilities.md b/docs/gdal-modern-capabilities.md index ca278d8..952ade3 100644 --- a/docs/gdal-modern-capabilities.md +++ b/docs/gdal-modern-capabilities.md @@ -55,6 +55,12 @@ multidimensional API. `TakeOutputDataset()` remains the runner's ownership-transfer method. Snapshots keep their diagnostics when a later compatibility operation changes the object's state. +Persistent outputs that the runner owns pass a checked close before success. +The runner reopens them read-only for `TakeOutputDataset()`. +Anonymous outputs remain open so in-memory pipelines retain their sources. +Caller-supplied output datasets remain caller-owned. The runner checks their +flush, but the caller controls their final close. + `Succeeded()` is true only after execution and finalization succeed. `LastErrorCode()` and `ErrorMsg()` select the last failure or fatal diagnostic. They return zero and an empty string when the diagnostics contain only warnings. @@ -88,6 +94,8 @@ Callers must still serialize access to shared datasets and process-wide configur Progress and logging callbacks run synchronously on the thread that executes the operation. Callbacks must not start another GDAL operation or wait for a thread that needs the execution lock. +The legacy warp utility rejects `-multi` with a progress callback to keep +this thread contract. The native operation APIs reject same-thread callback re-entry with `CoreError`. A callback must remain valid until the operation returns. Removing a logger does not stop a callback that already holds a copy of that logger. diff --git a/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp b/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp index c0f4ecc..dbd6064 100644 --- a/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp +++ b/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp @@ -233,6 +233,12 @@ namespace mwg::activex return E_FAIL; } + if (IntersectClipRect( + deviceContext, bounds.left, bounds.top, bounds.right, bounds.bottom) == ERROR) + { + return E_FAIL; + } + const ScopedGdiObject brush(CreateSolidBrush(kFillColor)); const ScopedGdiObject pen(CreatePen(PS_SOLID, 1, kOutlineColor)); if (brush.Get() == nullptr || pen.Get() == nullptr) diff --git a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h index 16dabcd..17e2328 100644 --- a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h +++ b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h @@ -55,6 +55,8 @@ namespace mwg [[nodiscard]] bool SetArgument(std::string_view name, const std::vector& value); /// Sets a dataset or dataset-list argument and retains a reference. /// Dataset lists receive one value. The dataset remains owned by the caller. + /// When used for "output", Run() checks a flush but does not explicitly + /// close the supplied dataset; GDAL reference ownership controls its close. [[nodiscard]] bool SetDatasetArgument(std::string_view name, const GdalDatasetPtr& value); /// Validates configured values using the instantiated GDAL algorithm's @@ -78,8 +80,14 @@ namespace mwg /// automatically; a later call is harmless and returns the same result. [[nodiscard]] bool Finalize(); - /// Transfers the output dataset to the caller, if the algorithm - /// produced one. The caller owns the returned dataset. + /// Transfers the runner's output reference to the caller, if the + /// algorithm produced one. Persistent outputs with a non-empty output + /// path or dataset description are checked and closed by Run(), then + /// reopened read-only; reopening failure makes Run() fail. + /// Anonymous outputs remain open to preserve driver dependencies. + /// For outputs supplied through SetDatasetArgument(), this is an + /// additional reference; the original remains caller-owned. Run() + /// checks its flush but cannot check its eventual close. [[nodiscard]] GdalDatasetPtr TakeOutputDataset(); /// Returns all diagnostics captured since construction. diff --git a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h index d3a5f25..1c22f87 100644 --- a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h +++ b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h @@ -93,6 +93,8 @@ namespace mwg const std::vector& options); /// Legacy warp with synchronous progress and cancellation. + /// @note The exact GDAL option "-multi" is rejected when a progress + /// callback is supplied because GDAL may invoke it from worker threads. [[nodiscard]] bool GdalRasterWarp( const std::filesystem::path& sourceFilename, const std::filesystem::path& destinationFilename, diff --git a/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h b/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h index 3ca027e..4de4144 100644 --- a/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h +++ b/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h @@ -69,6 +69,12 @@ namespace mwg /// \throws CoreError when counting or reading features fails. [[nodiscard]] std::int64_t FeatureCount() const; + /// Layer extent, or empty when the layer has no non-empty geometries. + /// Explicit GDAL failures throw CoreError; an unavailable driver extent + /// without diagnostics falls back to a checked O(n) geometry scan. + /// After a geometry edit on drivers other than Shapefile, the next call + /// scans features; later calls use the cached result. + /// \throws CoreError when GDAL cannot determine the extent. [[nodiscard]] Extent GetExtent() const; [[nodiscard]] int FieldCount() const; @@ -96,6 +102,11 @@ namespace mwg /// Attribute and geometry edits. Each writes the feature back /// immediately; the file is complete once the dataset is closed. + /// Attribute edits preserve the layer extent cache. Shapefile geometry + /// edits recompute stored bounds immediately with an O(n) driver scan, + /// so shrinking bounds survive reopening. Other drivers use a lazy O(n) + /// scan on the next GetExtent() call. A stored-bounds recomputation + /// failure throws CoreError even though the geometry was already written. /// \throws CoreError when the dataset is read-only or the index is invalid. void SetCellString(int fieldIndex, std::int64_t featureIndex, const std::string& value); void SetCellDouble(int fieldIndex, std::int64_t featureIndex, double value); diff --git a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp index c8fd77f..9941984 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -50,6 +51,96 @@ namespace mwg }); } + GDALAlgorithm* GetActualAlgorithm(GDALAlgorithm* algorithm) + { + return algorithm != nullptr ? &algorithm->GetActualAlgorithm() : nullptr; + } + + struct PersistentOutput final + { + std::string path; + std::string driverName; + std::vector openOptions; + unsigned int openFlags = GDAL_OF_READONLY | GDAL_OF_VERBOSE_ERROR; + }; + + std::optional DescribePersistentOutput( + const GDALArgDatasetValue& output, + GDALDataset* dataset) + { + if (dataset == nullptr) + return std::nullopt; + + const char* const driverName = dataset->GetDriverName(); + if (driverName != nullptr && + (std::string_view(driverName) == "MEM" || + std::string_view(driverName) == "Memory")) + { + return std::nullopt; + } + + const std::string& outputName = output.GetName(); + const char* const description = dataset->GetDescription(); + const bool hasOutputName = output.IsNameSet() && !outputName.empty(); + const bool hasDescription = description != nullptr && description[0] != '\0'; + if (!hasOutputName && !hasDescription) + return std::nullopt; + + PersistentOutput result; + result.path = hasDescription ? description : outputName; + result.driverName = driverName != nullptr ? driverName : ""; + if (dataset->GetRasterCount() > 0) + result.openFlags |= GDAL_OF_RASTER; + if (dataset->GetLayerCount() > 0) + result.openFlags |= GDAL_OF_VECTOR; + if (dataset->GetRootGroup() != nullptr) + result.openFlags |= GDAL_OF_MULTIDIM_RASTER; + + if ((result.openFlags & (GDAL_OF_RASTER | GDAL_OF_VECTOR | + GDAL_OF_MULTIDIM_RASTER)) == 0) + { + GDALDriver* const driver = dataset->GetDriver(); + if (driver != nullptr) + { + if (driver->GetMetadataItem(GDAL_DCAP_RASTER) != nullptr) + result.openFlags |= GDAL_OF_RASTER; + if (driver->GetMetadataItem(GDAL_DCAP_VECTOR) != nullptr) + result.openFlags |= GDAL_OF_VECTOR; + if (driver->GetMetadataItem(GDAL_DCAP_MULTIDIM_RASTER) != nullptr) + result.openFlags |= GDAL_OF_MULTIDIM_RASTER; + } + } + + const GDALDataset* const readOnlyDataset = dataset; + for (const char* const* option = readOnlyDataset->GetOpenOptions(); + option != nullptr && *option != nullptr; + ++option) + { + result.openOptions.emplace_back(*option); + } + return result; + } + + GdalDatasetPtr ReopenPersistentOutput(const PersistentOutput& output) + { + if (output.driverName.empty() || output.path.empty()) + return {}; + + const char* const allowedDrivers[]{output.driverName.c_str(), nullptr}; + std::vector openOptions; + openOptions.reserve(output.openOptions.size() + 1); + for (const std::string& option : output.openOptions) + openOptions.push_back(option.c_str()); + openOptions.push_back(nullptr); + + return GdalDatasetPtr(static_cast( + GDALOpenEx(output.path.c_str(), + output.openFlags, + allowedDrivers, + output.openOptions.empty() ? nullptr : openOptions.data(), + nullptr))); + } + void AppendErrors(std::vector& target, const GdalErrorScope& source) { const auto& errors = source.Errors(); @@ -100,6 +191,7 @@ namespace mwg { std::unique_ptr algorithm; GdalDatasetPtr outputDataset; + GDALDataset* callerProvidedOutputDataset = nullptr; std::vector errors; bool configurationValid = true; bool runAttempted = false; @@ -306,6 +398,11 @@ namespace mwg std::string(name) + "'."); _impl->configurationValid = false; } + else if (argument->GetType() == GAAT_DATASET && + argument->GetName() == "output") + { + _impl->callerProvidedOutputDataset = value.get(); + } return succeeded; } @@ -399,15 +496,27 @@ namespace mwg // or threw after GDAL had already produced its output. _impl->executionSucceeded = succeeded; bool finalized = false; + std::optional persistentOutput; + GDALArgDatasetValue* outputValue = nullptr; if (succeeded) { - GDALAlgorithmArg* output = _impl->algorithm != nullptr - ? _impl->algorithm->GetArg("output") + GDALAlgorithm* const actualAlgorithm = GetActualAlgorithm(_impl->algorithm.get()); + GDALAlgorithmArg* output = actualAlgorithm != nullptr + ? actualAlgorithm->GetArg("output") : nullptr; if (output != nullptr && output->GetType() == GAAT_DATASET) { - _impl->outputDataset.reset( - output->Get().GetDatasetIncreaseRefCount()); + outputValue = &output->Get(); + GDALDataset* const outputDataset = outputValue->GetDatasetRef(); + persistentOutput = DescribePersistentOutput(*outputValue, outputDataset); + const bool callerProvidedOutput = + outputDataset != nullptr && + outputDataset == _impl->callerProvidedOutputDataset; + if (!persistentOutput || callerProvidedOutput) + { + _impl->outputDataset.reset( + outputValue->GetDatasetIncreaseRefCount()); + } } finalized = Finalize(); } @@ -426,6 +535,35 @@ namespace mwg "GDAL algorithm output could not be flushed."); } } + else if (succeeded && persistentOutput.has_value() && outputValue != nullptr) + { + GdalErrorScope outputScope; + const std::size_t outputErrorCount = _impl->errors.size(); + const bool outputClosed = outputValue->Close(); + AppendErrors(_impl->errors, outputScope); + outputScope.RethrowCaptureException(); + if (!outputClosed) + { + AddFailureIfNoDiagnostic( + _impl->errors, + outputErrorCount, + "GDAL algorithm output could not be closed."); + } + else + { + GdalErrorScope reopenScope; + _impl->outputDataset = ReopenPersistentOutput(*persistentOutput); + AppendErrors(_impl->errors, reopenScope); + reopenScope.RethrowCaptureException(); + if (_impl->outputDataset == nullptr) + { + AddError( + _impl->errors, + "GDAL algorithm output could not be reopened read-only: " + + persistentOutput->path); + } + } + } scope.RethrowCaptureException(); bool callbackFailed = false; @@ -462,8 +600,9 @@ namespace mwg GdalErrorScope closeScope; const std::size_t closeErrorCount = _impl->errors.size(); _impl->outputDataset.reset(); - GDALAlgorithmArg* output = _impl->algorithm != nullptr - ? _impl->algorithm->GetArg("output") + GDALAlgorithm* const actualAlgorithm = GetActualAlgorithm(_impl->algorithm.get()); + GDALAlgorithmArg* output = actualAlgorithm != nullptr + ? actualAlgorithm->GetArg("output") : nullptr; bool outputClosed = true; if (output != nullptr && output->GetType() == GAAT_DATASET) diff --git a/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp b/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp index b8e31be..b1dd26b 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp @@ -447,6 +447,13 @@ namespace mwg _impl->cancelled = false; if (HasNullByte(options)) return RecordFailure(_impl->errors, "GDAL utility options must not contain null bytes."); + if (progress && + std::find(options.begin(), options.end(), "-multi") != options.end()) + { + return RecordFailure( + _impl->errors, + "GDAL raster warp option '-multi' is incompatible with synchronous progress callbacks."); + } return _impl->succeeded = RunUtilityOperation( _impl->errors, _impl->cancelled, std::move(progress), cancellationToken, diff --git a/src/MapWinGIS.Core/src/vector/VectorDataset.cpp b/src/MapWinGIS.Core/src/vector/VectorDataset.cpp index a18b200..6370ae1 100644 --- a/src/MapWinGIS.Core/src/vector/VectorDataset.cpp +++ b/src/MapWinGIS.Core/src/vector/VectorDataset.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -141,8 +142,8 @@ namespace mwg class VectorLayer::Impl { public: - Impl(OGRLayer* layer, const bool writable) - : _layer(layer), _writable(writable) + Impl(GDALDataset& dataset, OGRLayer* layer, const bool writable) + : _dataset(dataset), _layer(layer), _writable(writable) { if (const OGRFeatureDefn* defn = _layer->GetLayerDefn()) { @@ -189,16 +190,53 @@ namespace mwg return GetFeature(index); } - void Save(OGRFeature* feature) const + void Save(OGRFeature* feature, const bool geometryChanged = false) const { + GdalEnvironment::ClearLastError(); if (_layer->SetFeature(feature) != OGRERR_NONE) { - std::string message = "Cannot write feature " + std::to_string(feature->GetFID()); - if (const std::string detail = GdalEnvironment::LastErrorMessage(); !detail.empty()) + const GIntBig featureId = feature->GetFID(); + _cachedFeature.reset(); + _cachedIndex = -1; + _featureIds.clear(); + _featureIdsValid = false; + _featureCount.reset(); + if (geometryChanged) + { + _extent.reset(); + _extentNeedsExactRecalculation = true; + } + + std::string message = "Cannot write feature " + std::to_string(featureId); + if (const std::string detail = CPLGetLastErrorMsg(); !detail.empty()) message += " (" + detail + ")"; throw CoreError(message); } - _extent.reset(); + if (geometryChanged) + { + _extent.reset(); + _extentNeedsExactRecalculation = true; + if (const GDALDriver* driver = _dataset.GetDriver(); + driver != nullptr && + std::string_view(driver->GetDescription()) == "ESRI Shapefile") + { + // Shapefile SetFeature expands stored bounds but does not shrink them. + const std::string sql = + "RECOMPUTE EXTENT ON " + std::string(_layer->GetName()); + GdalEnvironment::ClearLastError(); + OGRLayer* result = _dataset.ExecuteSQL(sql.c_str(), nullptr, nullptr); + const CPLErr errorType = CPLGetLastErrorType(); + const std::string detail = CPLGetLastErrorMsg(); + if (result != nullptr) + _dataset.ReleaseResultSet(result); + if (errorType >= CE_Failure) + { + throw CoreError("Geometry was written, but cannot recompute stored " + "shapefile extent: " + detail); + } + _extentNeedsExactRecalculation = false; + } + } } [[nodiscard]] std::int64_t FeatureCount() const @@ -228,14 +266,38 @@ namespace mwg { if (!_extent) { - OGREnvelope envelope; - _extent = _layer->GetExtent(&envelope, TRUE) == OGRERR_NONE - ? Extent{envelope.MinX, envelope.MinY, envelope.MaxX, envelope.MaxY} - : Extent{}; + if (_extentNeedsExactRecalculation) + { + _extent = ComputeExactExtent(); + _extentNeedsExactRecalculation = false; + } + else + { + GdalEnvironment::ClearLastError(); + OGREnvelope envelope; + if (_layer->GetExtent(&envelope, TRUE) == OGRERR_NONE) + { + _extent = Extent{envelope.MinX, envelope.MinY, envelope.MaxX, envelope.MaxY}; + } + else + { + const std::string detail = CPLGetLastErrorMsg(); + const CPLErr errorType = CPLGetLastErrorType(); + if (errorType >= CE_Failure || !detail.empty()) + { + std::string message = "Cannot determine vector layer extent"; + if (!detail.empty()) + message += ": " + detail; + throw CoreError(message); + } + _extent = ComputeExactExtent(); + } + } } return *_extent; } + GDALDataset& _dataset; OGRLayer* _layer; bool _writable; std::vector _fields; @@ -285,6 +347,28 @@ namespace mwg _featureIdsValid = true; } + [[nodiscard]] Extent ComputeExactExtent() const + { + OGRLayer& layer = *_layer; + layer.ResetReading(); + const ReadingReset readingReset{layer}; + + std::optional extent; + while (const OGRFeatureUniquePtr feature{ReadNextFeature(layer)}) + { + const OGRGeometry* geometry = feature->GetGeometryRef(); + if (geometry == nullptr || geometry->IsEmpty()) + continue; + + OGREnvelope envelope; + geometry->getEnvelope(&envelope); + const Extent featureExtent{ + envelope.MinX, envelope.MinY, envelope.MaxX, envelope.MaxY}; + extent = extent ? extent->Union(featureExtent) : featureExtent; + } + return extent.value_or(Extent{}); + } + /// Tells the driver to skip attributes and geometry for the duration of /// the id scan, and restores the layer afterwards. class IgnoreAllButIds @@ -321,6 +405,7 @@ namespace mwg mutable std::int64_t _cachedIndex = -1; mutable std::optional _featureCount; mutable std::optional _extent; + mutable bool _extentNeedsExactRecalculation = false; }; std::int64_t detail::VectorLayerAccess::ForEachGeometry( @@ -545,7 +630,7 @@ namespace mwg if (feature->SetGeometryDirectly(parsed) != OGRERR_NONE) throw CoreError("Cannot assign geometry to feature"); - _impl->Save(feature); + _impl->Save(feature, true); } // -------------------------------------------------------------- Dataset @@ -562,7 +647,7 @@ namespace mwg { OGRLayer* layer = _dataset->GetLayer(i); _layers.push_back(std::unique_ptr( - new VectorLayer(std::make_unique(layer, writable)))); + new VectorLayer(std::make_unique(*_dataset, layer, writable)))); } } diff --git a/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp b/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp index f9ceecc..aef875b 100644 --- a/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp +++ b/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp @@ -553,6 +553,42 @@ TEST(VectorLayerRenderer, DrawsIntoBoundsWithAnOffsetOrigin) EXPECT_EQ(GetPixel(surface.Get(), 10, 10), GetSysColor(COLOR_WINDOW)); } +TEST(VectorLayerRenderer, GdiClipsCrossingPolygonsAndRestoresTheHostClip) +{ + for (const bool hasHostClip : {false, true}) + { + SCOPED_TRACE(hasHostClip); + BitmapSurface surface(200, 160); + ASSERT_TRUE(surface.IsValid()); + ASSERT_TRUE(PatBlt(surface.Get(), 0, 0, 200, 160, WHITENESS)); + if (hasHostClip) + { + ASSERT_NE(IntersectClipRect(surface.Get(), 20, 20, 120, 150), ERROR); + } + RECT originalClip{}; + ASSERT_NE(GetClipBox(surface.Get(), &originalClip), ERROR); + + const RECT bounds{40, 30, 160, 130}; + mwg::Viewport viewport(120, 100); + viewport.SetView({60.0, 50.0}, 1.0); + const auto scene = MakeMapScene({MakeRectangleLayer({-50.0, -50.0, 170.0, 150.0})}); + mwg::activex::GdiVectorLayerRenderer gdi; + ASSERT_EQ(gdi.Draw(surface.Get(), bounds, scene, viewport), S_OK); + + RECT restoredClip{}; + ASSERT_NE(GetClipBox(surface.Get(), &restoredClip), ERROR); + EXPECT_TRUE(EqualRect(&originalClip, &restoredClip)); + // Remove the host clip so samples can also inspect protected pixels. + ASSERT_NE(SelectClipRgn(surface.Get(), nullptr), ERROR); + EXPECT_EQ(GetPixel(surface.Get(), 60, 60), kFill); + EXPECT_EQ(GetPixel(surface.Get(), 39, 60), RGB(255, 255, 255)); + EXPECT_EQ(GetPixel(surface.Get(), 160, 60), RGB(255, 255, 255)); + EXPECT_EQ(GetPixel(surface.Get(), 60, 29), RGB(255, 255, 255)); + EXPECT_EQ(GetPixel(surface.Get(), 60, 130), RGB(255, 255, 255)); + EXPECT_EQ(GetPixel(surface.Get(), 125, 60), hasHostClip ? RGB(255, 255, 255) : kFill); + } +} + TEST(VectorLayerRenderer, GdiAndOpenGlAgreeAtFractionalRectangleEdges) { BitmapSurface gdiSurface(120, 100); diff --git a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp index 87ef2ed..b40cdae 100644 --- a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp @@ -236,7 +236,10 @@ namespace CPLErr Close() override { - return ReportFailure(_failClose, "Simulated output close failure"); + const CPLErr flushResult = FlushCache(false); + return flushResult == CE_None + ? ReportFailure(_failClose, "Simulated output close failure") + : flushResult; } private: @@ -261,6 +264,7 @@ namespace public: static GDALDataset* outputDataset; static bool runSucceeded; + static bool rejectOpen; OutputFailureAlgorithm() : GDALAlgorithm( @@ -291,8 +295,33 @@ namespace GDALArgDatasetValue _output; }; + class CallerOwnedOutputAlgorithm final : public GDALAlgorithm + { + public: + CallerOwnedOutputAlgorithm() + : GDALAlgorithm( + "mwg-test-caller-owned-output", + "Test caller-owned algorithm output", + "") + { + AddArg("output", 0, "Output dataset", &_output); + } + + protected: + bool RunImpl(GDALProgressFunc, void*) override + { + GDALDataset* const output = _output.GetDatasetRef(); + return output != nullptr && output->GetRasterBand(1) != nullptr && + output->GetRasterBand(1)->Fill(42) == CE_None; + } + + private: + GDALArgDatasetValue _output; + }; + GDALDataset* OutputFailureAlgorithm::outputDataset = nullptr; bool OutputFailureAlgorithm::runSucceeded = false; + bool OutputFailureAlgorithm::rejectOpen = false; void RegisterTestAlgorithms() { @@ -314,6 +343,8 @@ namespace [] { return std::make_unique(); }); registry.DeclareAlgorithm({"mwg-test", "output-failure"}, [] { return std::make_unique(); }); + registry.DeclareAlgorithm({"mwg-test", "caller-owned-output"}, + [] { return std::make_unique(); }); }); } @@ -654,8 +685,15 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS { return nullptr; } + if (OutputFailureAlgorithm::rejectOpen) + { + CPLError(CE_Failure, CPLE_OpenFailed, "%s", + "Simulated output reopen failure"); + return nullptr; + } const bool failFlush = path.ends_with(":flush"); - return new FailingOutputDataset(failFlush, !failFlush); + const bool failClose = path.ends_with(":close"); + return new FailingOutputDataset(failFlush, failClose); }; GetGDALDriverManager()->RegisterDriver(driver.get()); @@ -664,10 +702,14 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS const char* kind; const char* diagnostic; bool runSucceeded; + bool useGroupCommandLine; + bool rejectReopen; }; for (const FailureCase& failure : { - FailureCase{"flush", "Simulated output flush failure", true}, - FailureCase{"close", "Simulated output close failure", false}}) + FailureCase{"flush", "Simulated output flush failure", true, false, false}, + FailureCase{"close", "Simulated output close failure", true, false, false}, + FailureCase{"close", "Simulated output close failure", false, true, false}, + FailureCase{"reopen", "could not be reopened read-only", true, false, true}}) { const std::string path = std::string("MWG_OUTPUT_FAILURE:output:") + failure.kind; @@ -676,9 +718,18 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS ASSERT_NE(failingOutput, nullptr); OutputFailureAlgorithm::outputDataset = failingOutput.release(); OutputFailureAlgorithm::runSucceeded = failure.runSucceeded; - GdalAlgorithmRunner failingRunner("mwg-test output-failure"); + OutputFailureAlgorithm::rejectOpen = failure.rejectReopen; + GdalAlgorithmRunner failingRunner( + failure.useGroupCommandLine ? "mwg-test" : "mwg-test output-failure"); + if (failure.useGroupCommandLine) + { + ASSERT_TRUE(failingRunner.ParseCommandLineArgs({ + "output-failure", "--output", path + })) << LastError(failingRunner); + } EXPECT_FALSE(failingRunner.Run()); + OutputFailureAlgorithm::rejectOpen = false; EXPECT_NE( failingRunner.Result().ErrorMsg().find(failure.diagnostic), std::string::npos) @@ -740,6 +791,7 @@ TEST(GdalAlgorithmRunner, ConvertsGeoTiffEquivalentlyFromTypedAndCommandLineArgu ASSERT_NE(output, nullptr); EXPECT_EQ(output->GetRasterXSize(), 32); EXPECT_EQ(output->GetRasterYSize(), 24); + EXPECT_EQ(output->GetAccess(), GA_ReadOnly); output.reset(); GdalAlgorithmRunner parsed("raster convert"); @@ -760,3 +812,54 @@ TEST(GdalAlgorithmRunner, ConvertsGeoTiffEquivalentlyFromTypedAndCommandLineArgu EXPECT_EQ(typedDataset->GetRasterYSize(), parsedDataset->GetRasterYSize()); EXPECT_EQ(ReadPixels(typedDataset.get()), ReadPixels(parsedDataset.get())); } + +TEST(GdalAlgorithmRunner, CallerOwnedPersistentOutputRemainsOpenAndIsReturned) +{ + GdalEnvironment::Initialize(); + GdalEnvironment::QuietScope quiet; + test::TempDirectory directory; + const std::filesystem::path outputPath = directory.Path() / "caller-owned.tif"; + GDALDriver* const driver = GetGDALDriverManager()->GetDriverByName("GTiff"); + ASSERT_NE(driver, nullptr); + GdalDatasetPtr callerOutput(driver->Create( + ToUtf8(outputPath).c_str(), 32, 24, 1, GDT_Byte, nullptr)); + ASSERT_NE(callerOutput, nullptr); + GDALDataset* const originalOutput = callerOutput.get(); + + GdalDatasetPtr returnedOutput; + { + GdalAlgorithmRunner runner("mwg-test caller-owned-output"); + ASSERT_TRUE(runner.SetDatasetArgument("output", callerOutput)); + ASSERT_TRUE(runner.Run()) << LastError(runner); + returnedOutput = runner.TakeOutputDataset(); + } + + ASSERT_NE(returnedOutput, nullptr); + EXPECT_EQ(returnedOutput.get(), originalOutput); + EXPECT_EQ(returnedOutput->GetAccess(), GA_Update); + std::uint8_t pixel = 0; + ASSERT_EQ(returnedOutput->GetRasterBand(1)->RasterIO( + GF_Read, 0, 0, 1, 1, &pixel, 1, 1, GDT_Byte, 0, 0), + CE_None); + EXPECT_EQ(pixel, 42); +} + +TEST(GdalAlgorithmRunner, GroupCommandLineReturnsSelectedSubalgorithmMemoryOutput) +{ + GdalEnvironment::Initialize(); + GdalEnvironment::QuietScope quiet; + mwg::test::TempDirectory directory; + const std::filesystem::path input = MakeRaster(directory.Path(), "input.tif"); + + GdalAlgorithmRunner runner("raster"); + ASSERT_TRUE(runner.ParseCommandLineArgs({ + "convert", "--input", ToUtf8(input), "--output", "memory-output", + "--output-format", "MEM" + })) << LastError(runner); + ASSERT_TRUE(runner.Run()) << LastError(runner); + + GdalDatasetPtr output = runner.TakeOutputDataset(); + ASSERT_NE(output, nullptr); + EXPECT_EQ(output->GetRasterXSize(), 32); + EXPECT_EQ(output->GetRasterYSize(), 24); +} diff --git a/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp b/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp index 4ba1a69..1006e6e 100644 --- a/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp @@ -14,8 +14,10 @@ #include #include +#include #include "mapwingis/gdal/GdalEnvironment.h" +#include "TestData.h" using namespace mwg; @@ -81,6 +83,27 @@ TEST(GdalEnvironment, DeploysRuntimeDataNextToTheModule) << "proj.db is missing from " << dir.string(); } +#ifdef MWG_TEST_FULL_DRIVERS +TEST(GdalEnvironment, IncludesTheReleaseDriverSet) +{ + EXPECT_TRUE(GdalEnvironment::HasRasterDriver("HDF5")); + EXPECT_TRUE(GdalEnvironment::HasRasterDriver("netCDF")); + EXPECT_TRUE(GdalEnvironment::HasVectorDriver("LIBKML")); + EXPECT_TRUE(GdalEnvironment::HasVectorDriver("PostgreSQL")); + + GDALDriver* const sqlite = GetGDALDriverManager()->GetDriverByName("SQLite"); + ASSERT_NE(sqlite, nullptr); + const test::TempDirectory temp; + const std::string path = (temp.Path() / "spatialite.sqlite").string(); + char spatialiteOption[] = "SPATIALITE=YES"; + char* options[] = {spatialiteOption, nullptr}; + GDALDataset* const dataset = sqlite->Create( + path.c_str(), 0, 0, 0, GDT_Unknown, options); + ASSERT_NE(dataset, nullptr) << GdalEnvironment::LastErrorMessage(); + EXPECT_EQ(GDALClose(dataset), CE_None); +} +#endif + TEST(GdalEnvironment, CapturesTheLastError) { GdalEnvironment::Initialize(); diff --git a/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp b/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp index 793a8a0..f76fa4d 100644 --- a/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp @@ -10,9 +10,11 @@ #include #include +#include #include #include #include +#include #include #include #include @@ -261,6 +263,37 @@ TEST(GdalUtils, WarpsRasterUsingLegacyOptions) EXPECT_STREQ(dataset->GetSpatialRef()->GetAuthorityCode(nullptr), "3857"); } +TEST(GdalUtils, RejectsMultiWarpWhenSynchronousProgressCallbackIsProvided) +{ + GdalEnvironment::Initialize(); + GdalEnvironment::QuietScope quiet; + test::TempDirectory directory; + const std::filesystem::path input = test::MakeRaster(directory.Path()); + const std::filesystem::path rejectedOutput = directory.Path() / "rejected.tif"; + const std::filesystem::path noCallbackOutput = directory.Path() / "multi.tif"; + const std::thread::id callerThread = std::this_thread::get_id(); + std::atomic_int callbackCount = 0; + std::atomic_bool callbackRanOffThread = false; + GdalUtils utils; + + EXPECT_FALSE(utils.GdalRasterWarp( + input, rejectedOutput, {"-multi"}, + [&](double, std::string_view) + { + ++callbackCount; + callbackRanOffThread.store(std::this_thread::get_id() != callerThread); + return true; + })); + EXPECT_EQ(callbackCount.load(), 0); + EXPECT_FALSE(callbackRanOffThread.load()); + EXPECT_NE(utils.Result().ErrorMsg().find("'-multi'"), std::string::npos); + EXPECT_NE(utils.Result().ErrorMsg().find("synchronous"), std::string::npos); + EXPECT_FALSE(std::filesystem::exists(rejectedOutput)); + + EXPECT_TRUE(utils.GdalRasterWarp(input, noCallbackOutput, {"-multi"}, {}, nullptr)) + << utils.DetailedErrorMsg(); +} + TEST(GdalUtils, TranslatesVectorAndPreservesFeatures) { GdalEnvironment::Initialize(); diff --git a/tests/MapWinGIS.Core.Tests/MapWinGIS.Core.Tests.vcxproj b/tests/MapWinGIS.Core.Tests/MapWinGIS.Core.Tests.vcxproj index 729eb3a..24d9cb7 100644 --- a/tests/MapWinGIS.Core.Tests/MapWinGIS.Core.Tests.vcxproj +++ b/tests/MapWinGIS.Core.Tests/MapWinGIS.Core.Tests.vcxproj @@ -48,6 +48,12 @@ + + + MWG_TEST_FULL_DRIVERS;%(PreprocessorDefinitions) + + + diff --git a/tests/MapWinGIS.Core.Tests/TestData.cpp b/tests/MapWinGIS.Core.Tests/TestData.cpp index fa3bf6a..590ccd4 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.cpp +++ b/tests/MapWinGIS.Core.Tests/TestData.cpp @@ -46,6 +46,10 @@ namespace mwg::test { _definition->Reference(); _definition->SetGeomType(wkbPolygon); + OGRFieldDefn nameField("NAME", OFTString); + _definition->AddFieldDefn(&nameField); + OGRFieldDefn valueField("VALUE", OFTInteger); + _definition->AddFieldDefn(&valueField); } ~VectorReadTestLayer() override @@ -67,7 +71,7 @@ namespace mwg::test "Simulated feature read failure"); return nullptr; } - if (_state.nextFeature == 3) + if (_state.emptyLayer || _state.nextFeature == 3) { return nullptr; } @@ -78,10 +82,12 @@ namespace mwg::test { OGRFeatureUniquePtr feature(OGRFeature::CreateFeature(_definition)); feature->SetFID(id); + feature->SetField(0, _state.storedName.c_str()); + feature->SetField(1, _state.storedValue); if (!_state.ignoredFields && id != 1) { OGRPolygon polygon; - if (id == 0) + if (id == 0 && !_state.onlyEmptyGeometries) { OGRLinearRing ring; ring.addPoint(0.0, 0.0); @@ -102,7 +108,7 @@ namespace mwg::test GIntBig GetFeatureCount(int) override { ++_state.countCalls; - return _state.unknownCount ? -1 : 3; + return _state.unknownCount ? -1 : (_state.emptyLayer ? 0 : 3); } OGRErr SetIgnoredFields(CSLConstList fields) override @@ -116,14 +122,40 @@ namespace mwg::test return _definition; } - int TestCapability(const char*) const override + int TestCapability(const char* capability) const override { - return FALSE; + return capability != nullptr && + std::string_view(capability) == OLCRandomWrite + ? TRUE + : FALSE; } protected: + OGRErr ISetFeature(OGRFeature* feature) override + { + if (_state.failSetFeature) + { + CPLError(CE_Failure, CPLE_AppDefined, + "Simulated feature write failure"); + return OGRERR_FAILURE; + } + _state.storedName = feature->GetFieldAsString(0); + _state.storedValue = feature->GetFieldAsInteger(1); + return OGRERR_NONE; + } + OGRErr IGetExtent(int, OGREnvelope* extent, bool) override { + ++_state.extentCalls; + if (_state.failExtent) + { + CPLError(CE_Failure, CPLE_AppDefined, + "Simulated extent failure"); + return OGRERR_FAILURE; + } + if (_state.silentExtentFailure || _state.emptyLayer || + _state.onlyEmptyGeometries) + return OGRERR_FAILURE; extent->MinX = 0.0; extent->MinY = 0.0; extent->MaxX = 1.0; @@ -166,6 +198,7 @@ namespace mwg::test _driver->SetDescription("MWGVectorReadTest"); _driver->SetMetadataItem(GDAL_DCAP_VECTOR, "YES"); _driver->SetMetadataItem(GDAL_DCAP_OPEN, "YES"); + _driver->SetMetadataItem(GDAL_DCAP_UPDATE, "YES"); _driver->pDriverData = this; _driver->pfnIdentify = [](GDALOpenInfo* info) { diff --git a/tests/MapWinGIS.Core.Tests/TestData.h b/tests/MapWinGIS.Core.Tests/TestData.h index f32a506..cf3fe2f 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.h +++ b/tests/MapWinGIS.Core.Tests/TestData.h @@ -22,7 +22,8 @@ class GDALDriver; namespace mwg::test { /// Registers a synthetic polygon driver. Destroy opened datasets before - /// this helper. Its three features have polygon, null, and empty geometry. + /// this helper. Its three features have polygon, null, and empty geometry; + /// it can also simulate extent and feature-write failures. class VectorReadTestDriver { public: @@ -34,11 +35,19 @@ namespace mwg::test static constexpr const char* kPath = "MWG_VECTOR_READ_TEST:features"; bool failOnSecondRead = false; + bool failExtent = false; + bool silentExtentFailure = false; + bool failSetFeature = false; + bool emptyLayer = false; + bool onlyEmptyGeometries = false; bool unknownCount = false; bool ignoredFields = false; + std::string storedName = "Original"; + int storedValue = 0; int nextFeature = 0; int readCalls = 0; int countCalls = 0; + int extentCalls = 0; private: std::unique_ptr _driver; diff --git a/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp b/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp index afb4a62..a952e38 100644 --- a/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp @@ -112,6 +112,97 @@ TEST(VectorLayer, DoesNotCacheAnIncompleteFallbackFeatureCount) EXPECT_EQ(layer.FeatureCount(), 3); } +TEST(VectorLayer, RetriesTheExtentAfterAReadFailure) +{ + VectorReadTestDriver driver; + driver.failExtent = true; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath); + const VectorLayer& layer = dataset->Layer(0); + const GdalEnvironment::QuietScope quiet; + + try + { + (void)layer.GetExtent(); + FAIL() << "Expected the extent query to report a read failure"; + } + catch (const CoreError& error) + { + EXPECT_NE(std::string(error.what()).find("Simulated extent failure"), + std::string::npos); + } + + driver.failExtent = false; + const Extent extent = layer.GetExtent(); + EXPECT_DOUBLE_EQ(extent.xMin, 0.0); + EXPECT_DOUBLE_EQ(extent.xMax, 1.0); + EXPECT_EQ(driver.extentCalls, 2); +} + +TEST(VectorLayer, ReportsAnEmptyLayerExtent) +{ + const TempDirectory dir; + const std::array noRectangles{}; + const auto path = MakeRectanglesDataset(dir.Path(), "empty.gpkg", noRectangles); + const auto dataset = VectorDataset::Open(path); + + EXPECT_TRUE(dataset->Layer(0).GetExtent().IsEmpty()); +} + +TEST(VectorLayer, ReportsExtentFailuresEvenWhenTheLayerHasNoFeatures) +{ + VectorReadTestDriver driver; + driver.emptyLayer = true; + driver.failExtent = true; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath); + const VectorLayer& layer = dataset->Layer(0); + EXPECT_EQ(layer.FeatureCount(), 0); + + const GdalEnvironment::QuietScope quiet; + try + { + (void)layer.GetExtent(); + FAIL() << "Expected an operational extent failure on the empty layer"; + } + catch (const CoreError& error) + { + EXPECT_NE(std::string(error.what()).find("Simulated extent failure"), + std::string::npos); + } + driver.failExtent = false; + EXPECT_TRUE(layer.GetExtent().IsEmpty()); + EXPECT_EQ(driver.extentCalls, 2); +} + +TEST(VectorLayer, ReportsAnEmptyExtentForNullAndEmptyGeometries) +{ + VectorReadTestDriver driver; + driver.onlyEmptyGeometries = true; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath); + const VectorLayer& layer = dataset->Layer(0); + + EXPECT_EQ(layer.FeatureCount(), 3); + EXPECT_TRUE(layer.GetExtent().IsEmpty()); + EXPECT_TRUE(layer.GetExtent().IsEmpty()); + EXPECT_EQ(driver.readCalls, 4); + EXPECT_EQ(driver.extentCalls, 1); +} + +TEST(VectorLayer, RetriesTheFallbackExtentScanAfterAReadFailure) +{ + VectorReadTestDriver driver; + driver.silentExtentFailure = true; + driver.failOnSecondRead = true; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath); + const VectorLayer& layer = dataset->Layer(0); + const GdalEnvironment::QuietScope quiet; + + EXPECT_THROW((void)layer.GetExtent(), CoreError); + EXPECT_EQ(driver.nextFeature, 0); + driver.failOnSecondRead = false; + EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); + EXPECT_EQ(driver.extentCalls, 2); +} + TEST_P(VectorLayerTest, ReportsTheExtent) { const Extent extent = _layer->GetExtent(); @@ -408,6 +499,48 @@ TEST_P(VectorLayerEditTest, ReflectsEditsWithoutReopening) EXPECT_TRUE(layer.IsCellNull(area, 0)); } +TEST(VectorLayer, FailedFeatureWritesDoNotRemainInTheFeatureCache) +{ + VectorReadTestDriver driver; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); + VectorLayer& layer = dataset->Layer(0); + const int name = layer.FieldIndex("NAME"); + ASSERT_TRUE(layer.IsWritable()); + const GdalEnvironment::QuietScope quiet; + + driver.failSetFeature = true; + try + { + layer.SetCellString(name, 0, "Rejected"); + FAIL() << "Expected the feature write to fail"; + } + catch (const CoreError& error) + { + EXPECT_NE(std::string(error.what()).find("Simulated feature write failure"), + std::string::npos); + } + + EXPECT_EQ(layer.CellString(name, 0), "Original"); + driver.failSetFeature = false; + layer.SetCellInteger(layer.FieldIndex("VALUE"), 0, 42); + EXPECT_EQ(layer.CellString(name, 0), "Original"); + EXPECT_EQ(driver.storedName, "Original"); + EXPECT_EQ(driver.storedValue, 42); +} + +TEST(VectorLayer, AttributeEditsDoNotInvalidateTheExtent) +{ + VectorReadTestDriver driver; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); + VectorLayer& layer = dataset->Layer(0); + + (void)layer.GetExtent(); + layer.SetCellString(layer.FieldIndex("NAME"), 0, "Changed"); + (void)layer.GetExtent(); + + EXPECT_EQ(driver.extentCalls, 1); +} + TEST_P(VectorLayerEditTest, RecomputesTheExtentAfterAGeometryEdit) { static constexpr const char* kMoved = @@ -415,13 +548,40 @@ TEST_P(VectorLayerEditTest, RecomputesTheExtentAfterAGeometryEdit) const std::unique_ptr dataset = VectorDataset::Open(_path, OpenMode::Update); VectorLayer& layer = dataset->Layer(0); - ASSERT_DOUBLE_EQ(layer.GetExtent().xMax, Provinces::kMaxX); - layer.SetGeometryWkt(0, kMoved); EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 201000.0); } +TEST_P(VectorLayerEditTest, ShrinksTheExtentAfterAGeometryEdit) +{ + static constexpr const char* kMoved = + "POLYGON ((100500 400000, 101500 400000, 101500 401000, 100500 401000, 100500 400000))"; + + const std::unique_ptr dataset = VectorDataset::Open(_path, OpenMode::Update); + VectorLayer& layer = dataset->Layer(0); + + ASSERT_DOUBLE_EQ(layer.GetExtent().xMax, Provinces::kMaxX); + + layer.SetGeometryWkt(3, kMoved); + EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 103000.0); +} + +TEST(VectorLayer, PersistsShrinkingShapefileBoundsWithoutAnExtentQuery) +{ + const TempDirectory dir; + const auto path = MakeProvincesDataset(dir.Path(), "provinces.shp"); + { + const auto dataset = VectorDataset::Open(path, OpenMode::Update); + dataset->Layer(0).SetGeometryWkt( + 3, "POLYGON ((100500 400000, 101500 400000, 101500 401000, " + "100500 401000, 100500 400000))"); + } + + const auto reopened = VectorDataset::Open(path); + EXPECT_DOUBLE_EQ(reopened->Layer(0).GetExtent().xMax, 103000.0); +} + TEST_P(VectorLayerEditTest, KeepsGeneratedFixturesIndependent) { Edit([](VectorLayer& layer) From 9116c602887fbd68c000d7516f63181434ff71f7 Mon Sep 17 00:00:00 2001 From: Paul Meems Date: Fri, 6 Nov 2026 21:49:59 +0100 Subject: [PATCH 2/4] Address PR #60 review findings Fix output completion and access, batch persisted extents, protect threaded progress callbacks, and verify Release builds on pull requests. Update regression tests, documentation, and shared memory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .ai/memory/architecture/core-library.md | 5 +- .ai/memory/build/windows.md | 7 +- .ai/memory/lessons/lessons.md | 10 +- .github/workflows/build.yml | 3 +- Directory.Build.targets | 4 +- README.md | 6 + build/scripts/Test-BuildConfiguration.ps1 | 11 +- docs/ACTIVEX-INVESTIGATION.md | 4 +- docs/ARCHITECTURE.md | 20 +- docs/BUILDING.md | 5 +- docs/TESTING.md | 4 +- docs/gdal-integration.md | 18 +- docs/gdal-modern-capabilities.md | 14 +- .../GdiVectorLayerRenderer.cpp | 15 +- src/MapWinGIS.Core/MapWinGIS.Core.vcxproj | 1 + .../mapwingis/gdal/GdalAlgorithmRunner.h | 10 +- .../include/mapwingis/gdal/GdalProgress.h | 4 +- .../include/mapwingis/gdal/GdalUtils.h | 4 +- .../include/mapwingis/vector/VectorDataset.h | 25 +-- .../src/gdal/GdalAlgorithmRunner.cpp | 66 +++++-- .../src/gdal/GdalEnvironment.cpp | 5 +- src/MapWinGIS.Core/src/gdal/GdalRuntime.cpp | 6 +- src/MapWinGIS.Core/src/gdal/GdalUtils.cpp | 8 - .../src/vector/VectorDataset.cpp | 106 ++++++---- .../VectorLayerRendererTests.cpp | 26 +++ .../GdalAlgorithmRunnerTests.cpp | 184 +++++++++++++++++- .../GdalEnvironmentTests.cpp | 12 +- tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp | 41 ++-- tests/MapWinGIS.Core.Tests/TestData.cpp | 28 ++- tests/MapWinGIS.Core.Tests/TestData.h | 5 + .../MapWinGIS.Core.Tests/VectorLayerTests.cpp | 94 ++++++++- 31 files changed, 577 insertions(+), 174 deletions(-) diff --git a/.ai/memory/architecture/core-library.md b/.ai/memory/architecture/core-library.md index c1bbf9e..c9d067e 100644 --- a/.ai/memory/architecture/core-library.md +++ b/.ai/memory/architecture/core-library.md @@ -27,7 +27,10 @@ remain Core responsibilities; COM callbacks and host dispatch stay in ActiveX. Algorithm outputs need different completion rules. Runner-owned persistent -outputs pass a checked close and reopen read-only before success. Anonymous +outputs pass a checked close before success. After successful finalization, +the runner tries all readers with the original access mode, then read-only. +An unavailable reader produces a warning and no returned dataset, not a failed +write. Anonymous outputs retain their references for pipelines. Caller-supplied outputs remain caller-owned, so the runner can check their flush but not their final close. See `GdalAlgorithmRunner` and its output-failure and ownership tests. diff --git a/.ai/memory/build/windows.md b/.ai/memory/build/windows.md index 826f92d..a9f8e40 100644 --- a/.ai/memory/build/windows.md +++ b/.ai/memory/build/windows.md @@ -11,9 +11,10 @@ automated UI and pixel checks. ## CI scope -`.github/workflows/build.yml` runs Debug x64 build and native tests for pull -requests. Pushes to `main` additionally build and test Debug and Release for -both `x64` and `Win32`. The runner overrides the default SDK with `10.0` +`.github/workflows/build.yml` runs Debug and full-driver Release x64 builds +and native tests for pull requests and pushes to `development`. +Each job has a 180-minute timeout. Pushes to `main` additionally build and test +both configurations for `Win32`. The runner overrides the default SDK with `10.0` because the `windows-2025` image does not include the repository's pinned SDK. The C# host checks require an interactive desktop and are not part of this CI workflow. diff --git a/.ai/memory/lessons/lessons.md b/.ai/memory/lessons/lessons.md index 1e48804..846912e 100644 --- a/.ai/memory/lessons/lessons.md +++ b/.ai/memory/lessons/lessons.md @@ -54,10 +54,12 @@ and [PR #26](https://github.com/MapWindow/MapWinGIS6/pull/26). ## Shapefile geometry edits do not shrink stored bounds OGR's Shapefile driver expands bounds during `SetFeature` but does not shrink -them. Core runs `RECOMPUTE EXTENT ON` after each Shapefile geometry edit so -the stored extent remains correct after reopening. This costs an O(n) scan -per geometry edit. Attribute edits do not trigger it. A recomputation failure -throws after the geometry write succeeds, so callers must not assume rollback. +them. Core runs `RECOMPUTE EXTENT ON` during an explicit `VectorDataset::Flush()` +for dirty Shapefile and GeoPackage layers. Call it before closing after geometry +edits to preserve exact stored bounds. This costs one O(n) scan per dirty layer, +not per edit. Attribute edits do not trigger it. A recomputation failure throws +after geometry writes and leaves the layer dirty for retry. It does not roll +back the writes. Other formats retain their driver's stored-extent behavior. See [VectorDataset.cpp](../../../src/MapWinGIS.Core/src/vector/VectorDataset.cpp) and the shrinking and reopening cases in diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 858a4f1..5417172 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -12,10 +12,11 @@ concurrency: jobs: build: runs-on: windows-2025 + timeout-minutes: 180 strategy: fail-fast: false - matrix: ${{ fromJSON(github.event_name == 'push' && github.ref == 'refs/heads/main' && '{"platform":["x64","Win32"],"configuration":["Debug","Release"]}' || '{"include":[{"platform":"x64","configuration":"Debug"}]}') }} + matrix: ${{ fromJSON(github.event_name == 'push' && github.ref == 'refs/heads/main' && '{"platform":["x64","Win32"],"configuration":["Debug","Release"]}' || '{"platform":["x64"],"configuration":["Debug","Release"]}') }} env: VCPKG_BINARY_SOURCES: 'clear;files,${{ github.workspace }}\vcpkg-binary-cache,readwrite' diff --git a/Directory.Build.targets b/Directory.Build.targets index 5d97501..fb3d556 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -11,12 +11,12 @@ --> - true + false + Condition="'$(MwgDeployRuntimeData)' == 'true'"> diff --git a/README.md b/README.md index a3ac6c7..386d88b 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,12 @@ For additional configurations, dependency features, and troubleshooting, see CI Release artifacts use the `full-drivers` feature. Their tests require HDF5, NetCDF, KML, SpatiaLite, and PostgreSQL support. +Pull requests build and test both Debug and full-driver Release for x64. +Each CI build has a 180-minute timeout. + +After vector geometry edits, call `VectorDataset::Flush()` before closing to +persist exact Shapefile and GeoPackage bounds. The flush batches stored-extent +scans across multiple edits. ## Layout diff --git a/build/scripts/Test-BuildConfiguration.ps1 b/build/scripts/Test-BuildConfiguration.ps1 index 4d68e7b..cd36d89 100644 --- a/build/scripts/Test-BuildConfiguration.ps1 +++ b/build/scripts/Test-BuildConfiguration.ps1 @@ -11,9 +11,9 @@ $ErrorActionPreference = 'Stop' $repoRoot = (Resolve-Path (Join-Path $PSScriptRoot '..\..')).Path $temp = Join-Path ([System.IO.Path]::GetTempPath()) "MapWinGIS build checks $([guid]::NewGuid())" try { - New-Item -ItemType Directory -Path "$temp\build\props" -Force | Out-Null + New-Item -ItemType Directory -Path $temp -Force | Out-Null Copy-Item "$repoRoot\Directory.Build.props", "$repoRoot\Directory.Build.targets" $temp - Copy-Item "$repoRoot\build\props\MapWinGIS.Cpp.props" "$temp\build\props" + Copy-Item "$repoRoot\build" "$temp\build" -Recurse New-Item -ItemType Directory -Path "$temp\packages\share\gdal", "$temp\packages\share\proj" -Force | Out-Null Set-Content "$temp\packages\share\gdal\test.csv" 'GDAL fixture' Set-Content "$temp\packages\share\proj\proj.db" 'PROJ fixture' @@ -29,7 +29,6 @@ $commandLine | Select-Object -Skip $start | Set-Content (Join-Path $PSScriptRoot Debug x64 $(MSBuildThisFileDirectory)no-vcpkg - tests;full-drivers @@ -42,10 +41,10 @@ $commandLine | Select-Object -Skip $start | Set-Content (Join-Path $PSScriptRoot '@ - Set-Content "$temp\MapWinGIS.Core.proj" $project + Set-Content "$temp\RuntimeDataOwner.proj" $project Set-Content "$temp\MapWinGIS.ActiveX.proj" $project $powerShell = (Get-Process -Id $PID).Path - & $MSBuild "$temp\MapWinGIS.Core.proj" -nologo -v:q -t:CaptureArguments "-p:MwgTestPowerShell=$powerShell" + & $MSBuild "$temp\RuntimeDataOwner.proj" -nologo -v:q -t:CaptureArguments "-p:MwgTestPowerShell=$powerShell" '-p:MwgVcpkgFeatures=tests%3Bfull-drivers' if ($LASTEXITCODE -ne 0) { throw 'Dependency argument capture failed.' } $arguments = @(Get-Content "$temp\arguments.txt") if ($arguments.Count -ne 3 -or @@ -60,7 +59,7 @@ $commandLine | Select-Object -Skip $start | Set-Content (Join-Path $PSScriptRoot if ($LASTEXITCODE -ne 0) { throw 'Consumer deployment check failed.' } if (Test-Path "$temp\output") { throw 'A consumer copied shared runtime data.' } - & $MSBuild "$temp\MapWinGIS.Core.proj" -nologo -v:q -t:MwgDeployGdalRuntimeData + & $MSBuild "$temp\RuntimeDataOwner.proj" -nologo -v:q -t:MwgDeployGdalRuntimeData -p:MwgDeployRuntimeData=true if ($LASTEXITCODE -ne 0) { throw 'Core deployment failed.' } foreach ($relative in @('gdal-data\test.csv', 'proj\proj.db')) { if (-not (Test-Path "$temp\output\$relative")) { throw "Runtime data missing: $relative" } diff --git a/docs/ACTIVEX-INVESTIGATION.md b/docs/ACTIVEX-INVESTIGATION.md index 03d5e56..5d4415c 100644 --- a/docs/ACTIVEX-INVESTIGATION.md +++ b/docs/ACTIVEX-INVESTIGATION.md @@ -18,8 +18,8 @@ create a raster directly with GDAL, but this is test fixture code rather than a core raster API. GDAL must be initialized through `GdalEnvironment`. The build already defines Debug and Release for Win32 and x64 and uses vcpkg -for GDAL, GEOS, and GoogleTest. Pull requests build and test Debug x64; pushes -to `main` also cover Debug and Release for Win32 and x64. +for GDAL, GEOS, and GoogleTest. Pull requests build and test Debug and full-driver +Release for x64. Pushes to `main` also cover Win32. ## MapWinGIS 5 reference diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index ebafce2..ed1a4e8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -91,8 +91,9 @@ expect. `SetLogHandler()`. Never write to stderr and never show message boxes from the core. - Algorithm runners, compatibility utilities, and pipelines share an execution - lock. Their callbacks run on the calling thread and cannot re-enter these - operations. Direct dataset access still needs caller synchronization. + lock. GDAL can report progress on worker threads. The shared progress adapter + serializes callbacks, which cannot re-enter these operations. + Direct dataset access still needs caller synchronization. See [operation results and threading](gdal-modern-capabilities.md#operation-results-logging-and-cancellation) for result, cancellation, and callback lifetime contracts. - The supported GDAL version and dependency pin are documented in @@ -140,11 +141,16 @@ state; hosts and renderers read from it. Vector layer extent queries cache successful results. Read failures throw without caching an empty result. If a driver cannot provide an extent and -reports no error, Core scans the geometries with read-error checks. -Attribute edits keep the cached extent. Shapefile geometry edits immediately -recompute stored bounds with an O(n) scan, so smaller bounds survive reopening. -If that recomputation fails, the edit throws after writing the geometry. -For other drivers, the next extent query scans once after a geometry edit. +reports no failure diagnostic, Core scans the geometries with read-error checks. +The scan asks the driver to ignore attributes. +Attribute edits keep the cached extent. After geometry edits, the next extent +query scans once and caches the result. + +Call `VectorDataset::Flush()` before closing after geometry edits to persist +exact Shapefile and GeoPackage bounds. The flush runs one O(n) stored-extent +scan per dirty layer, rather than one per edit. A recomputation failure throws +without rolling back geometry writes and leaves the layer dirty for retry. +Other formats retain their driver's stored-extent behavior. ## Viewport diff --git a/docs/BUILDING.md b/docs/BUILDING.md index 43631a2..afd279a 100644 --- a/docs/BUILDING.md +++ b/docs/BUILDING.md @@ -168,6 +168,9 @@ msbuild MapWinGIS6.slnx -restore -p:Platform=x64 -p:Configuration=Release "-p:Mw CI uses this feature set for Release artifacts. Core tests built with `full-drivers` require HDF5, NetCDF, LIBKML, PostgreSQL, and SpatiaLite support. The SpatiaLite test creates a spatial database without an external server. +Pull requests and pushes to `development` build and test Debug and full-driver +Release for x64. Pushes to `main` also cover Win32. Each job has a +180-minute timeout. ### Runtime data @@ -216,7 +219,7 @@ bin\Release\x64\MapWinGIS.ActiveX.Tests.exe --gtest_output=xml:activex-testresul | `MwgVcpkgRoot` | resolved (see above) | vcpkg location | | `MwgVcpkgFeatures` | `tests` | Semicolon-separated manifest features | | `MwgEnableAsan` | `false` | Adds `/fsanitize=address` | -| `MwgDeployRuntimeData` | `true` | Copies `gdal-data` and `proj` to the output folder | +| `MwgDeployRuntimeData` | `true` for Core, `false` otherwise | Copies `gdal-data` and `proj` to the output folder | ## Troubleshooting diff --git a/docs/TESTING.md b/docs/TESTING.md index 233ca5c..8ee628e 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -30,10 +30,12 @@ Run the build configuration tests from a Visual Studio developer shell: pwsh -File build\scripts\Test-BuildConfiguration.ps1 ``` -These tests pass dependency arguments through MSBuild in a path with spaces. +These tests pass `tests%3Bfull-drivers` on the MSBuild command line in a path +with spaces and inspect the resulting dependency arguments. They also make sure that only Core deploys GDAL and PROJ data to the shared output folder. CI runs them before the solution build. Builds with `full-drivers` also test the release driver set in `GdalEnvironment` tests. +Pull requests run both Debug and full-driver Release tests for x64. ## ActiveX host integration checks diff --git a/docs/gdal-integration.md b/docs/gdal-integration.md index ee209cc..7f2f19d 100644 --- a/docs/gdal-integration.md +++ b/docs/gdal-integration.md @@ -42,8 +42,11 @@ against GDAL's metadata and captures warnings and failures in `Errors()`. `TakeOutputDataset()` transfers ownership of an output dataset. For persistent outputs that the runner owns, `Run()` checks the final close -before reporting success. It reopens the output read-only for ownership -transfer. Reopening failures also fail the operation. Anonymous outputs, +before reporting success. After successful finalization, it tries all registered +readers to reopen the output with its original access mode. +If update access fails, it tries read-only access. If no reader can open the +output, the write still succeeds with a warning and `TakeOutputDataset()` returns +`nullptr`. This includes write-only targets such as `/vsistdout/`. Anonymous outputs, including in-memory VRT datasets, remain open. If the caller supplies an output through `SetDatasetArgument()`, the runner checks its flush but cannot check its eventual close. The caller retains the original reference. @@ -77,7 +80,8 @@ copying an option list into application documentation. ## Progress and cancellation -Progress callbacks execute synchronously on the thread running GDAL. Return +GDAL can invoke progress callbacks on worker threads. The shared adapter +serializes callbacks and protects cancellation and exception state. Return `false` to cancel. A cancellation token can also be cancelled from another thread and must remain alive until the operation returns: @@ -98,9 +102,8 @@ algorithms promptly. Cancellation can leave partial output files. Callbacks must not start another GDAL operation or wait on a thread that needs the GDAL execution lock; callback exceptions are returned as operation failures. -The legacy `GdalUtils::GdalRasterWarp()` rejects `-multi` when a progress -callback is supplied. GDAL can call that callback from a worker thread with -this option. Calls without a progress callback can still use `-multi`. +The legacy `GdalUtils::GdalRasterWarp()` accepts `-multi` and threaded warp +options such as `-wo NUM_THREADS=2`, with or without a progress callback. ## ActiveX operations @@ -226,7 +229,8 @@ The solution builds the runnable C++ example direct GDAL calls are not covered by that lock. Synchronize access to shared datasets and process-wide configuration yourself. - Native Core objects are not thread-safe unless their API says otherwise. - Progress and logging callbacks run synchronously on the operation thread. + GDAL can report progress on worker threads. The adapter serializes progress + callbacks. Logging callbacks run on the thread that raises the diagnostic. - If initialization reports missing GDAL or PROJ data, rebuild with runtime data deployment enabled. The data directories are placed beside the DLL; do not set `GDAL_DATA` or `PROJ_LIB` as a workaround. diff --git a/docs/gdal-modern-capabilities.md b/docs/gdal-modern-capabilities.md index 952ade3..73d148e 100644 --- a/docs/gdal-modern-capabilities.md +++ b/docs/gdal-modern-capabilities.md @@ -56,7 +56,10 @@ multidimensional API. Snapshots keep their diagnostics when a later compatibility operation changes the object's state. Persistent outputs that the runner owns pass a checked close before success. -The runner reopens them read-only for `TakeOutputDataset()`. +After successful finalization, the runner tries all registered readers to +reopen them with their original access mode, then read-only if needed. +If no reader can open an output, the write still succeeds with a warning. +`TakeOutputDataset()` then returns `nullptr`. Anonymous outputs remain open so in-memory pipelines retain their sources. Caller-supplied output datasets remain caller-owned. The runner checks their flush, but the caller controls their final close. @@ -92,11 +95,12 @@ Their diagnostic lists remain separate. This lock does not cover direct GDAL calls, returned datasets, or the vector dataset API. Callers must still serialize access to shared datasets and process-wide configuration. -Progress and logging callbacks run synchronously on the thread that executes the operation. +GDAL can report progress on worker threads. The progress adapter serializes +callbacks and protects cancellation and exception state across threads. +Logging callbacks run on the thread that raises the diagnostic. Callbacks must not start another GDAL operation or wait for a thread that needs the execution lock. -The legacy warp utility rejects `-multi` with a progress callback to keep -this thread contract. -The native operation APIs reject same-thread callback re-entry with `CoreError`. +Threaded warp options remain supported with progress callbacks. +The native operation APIs reject callback re-entry with `CoreError` on every thread. A callback must remain valid until the operation returns. Removing a logger does not stop a callback that already holds a copy of that logger. Use owned state or shared ownership for captures that other threads can access. diff --git a/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp b/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp index dbd6064..a6a7458 100644 --- a/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp +++ b/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp @@ -233,11 +233,22 @@ namespace mwg::activex return E_FAIL; } - if (IntersectClipRect( - deviceContext, bounds.left, bounds.top, bounds.right, bounds.bottom) == ERROR) + const int clipResult = IntersectClipRect( + deviceContext, bounds.left, bounds.top, bounds.right, bounds.bottom); + if (clipResult == ERROR) { return E_FAIL; } + RECT clipBounds{}; + const int effectiveClip = GetClipBox(deviceContext, &clipBounds); + if (effectiveClip == ERROR) + { + return E_FAIL; + } + if (effectiveClip == NULLREGION) + { + return S_OK; + } const ScopedGdiObject brush(CreateSolidBrush(kFillColor)); const ScopedGdiObject pen(CreatePen(PS_SOLID, 1, kOutlineColor)); diff --git a/src/MapWinGIS.Core/MapWinGIS.Core.vcxproj b/src/MapWinGIS.Core/MapWinGIS.Core.vcxproj index 4e7f52c..23a6914 100644 --- a/src/MapWinGIS.Core/MapWinGIS.Core.vcxproj +++ b/src/MapWinGIS.Core/MapWinGIS.Core.vcxproj @@ -25,6 +25,7 @@ MapWinGISCore MapWinGIS.Core MapWinGIS.Core + true diff --git a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h index 17e2328..01d1531 100644 --- a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h +++ b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalAlgorithmRunner.h @@ -68,7 +68,8 @@ namespace mwg /// Runs and finalizes the configured algorithm, reporting progress and /// cancellation through the existing GDAL callback adapter. - /// Callbacks run on this thread and must not start GDAL operations or + /// Progress callbacks are serialized, but GDAL can invoke them on worker + /// threads. They must not start GDAL operations or /// wait for other threads that need GDAL. Callback exceptions become /// failure diagnostics, not user cancellation. Token/callback lifetimes /// must cover this call. Capture/logger exceptions propagate after @@ -82,8 +83,11 @@ namespace mwg /// Transfers the runner's output reference to the caller, if the /// algorithm produced one. Persistent outputs with a non-empty output - /// path or dataset description are checked and closed by Run(), then - /// reopened read-only; reopening failure makes Run() fail. + /// path or dataset description are checked and closed by Run(). + /// The runner then tries all registered readers to reopen the output + /// with its original access mode, falling back to read-only if needed. + /// If no reader can open it (including write-only streaming targets), + /// Run() still succeeds and records a warning, but this returns nullptr. /// Anonymous outputs remain open to preserve driver dependencies. /// For outputs supplied through SetDatasetArgument(), this is an /// additional reference; the original remains caller-owned. Run() diff --git a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalProgress.h b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalProgress.h index ddba53e..d481ff3 100644 --- a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalProgress.h +++ b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalProgress.h @@ -36,7 +36,9 @@ namespace mwg /// Adapts GDAL progress to a C++ callback and cancellation token. /// - /// Progress callbacks run synchronously on the thread executing GDAL. + /// Progress callbacks run synchronously on the GDAL thread that reports + /// progress, which can be a worker thread. The adapter serializes callbacks + /// and protects their cancellation and exception state across threads. /// Keep the adapter alive through the GDAL operation; its cancellation /// token must also outlive the operation. If a callback /// throws, the adapter cancels the operation and preserves the exception diff --git a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h index 1c22f87..3137653 100644 --- a/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h +++ b/src/MapWinGIS.Core/include/mapwingis/gdal/GdalUtils.h @@ -93,8 +93,8 @@ namespace mwg const std::vector& options); /// Legacy warp with synchronous progress and cancellation. - /// @note The exact GDAL option "-multi" is rejected when a progress - /// callback is supplied because GDAL may invoke it from worker threads. + /// GDAL can invoke progress on worker threads with "-multi" or + /// threaded warp options. The shared adapter serializes these callbacks. [[nodiscard]] bool GdalRasterWarp( const std::filesystem::path& sourceFilename, const std::filesystem::path& destinationFilename, diff --git a/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h b/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h index 4de4144..41ae5b4 100644 --- a/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h +++ b/src/MapWinGIS.Core/include/mapwingis/vector/VectorDataset.h @@ -71,8 +71,8 @@ namespace mwg /// Layer extent, or empty when the layer has no non-empty geometries. /// Explicit GDAL failures throw CoreError; an unavailable driver extent - /// without diagnostics falls back to a checked O(n) geometry scan. - /// After a geometry edit on drivers other than Shapefile, the next call + /// without failure diagnostics falls back to a checked O(n) geometry scan. + /// After a geometry edit, the next call /// scans features; later calls use the cached result. /// \throws CoreError when GDAL cannot determine the extent. [[nodiscard]] Extent GetExtent() const; @@ -101,12 +101,11 @@ namespace mwg [[nodiscard]] bool IsWritable() const; /// Attribute and geometry edits. Each writes the feature back - /// immediately; the file is complete once the dataset is closed. - /// Attribute edits preserve the layer extent cache. Shapefile geometry - /// edits recompute stored bounds immediately with an O(n) driver scan, - /// so shrinking bounds survive reopening. Other drivers use a lazy O(n) - /// scan on the next GetExtent() call. A stored-bounds recomputation - /// failure throws CoreError even though the geometry was already written. + /// immediately. Attribute edits preserve the layer extent cache. + /// Geometry edits use a lazy O(n) scan on the next GetExtent() call. + /// Call VectorDataset::Flush() before closing after geometry edits to + /// persist exact stored bounds for Shapefile and GeoPackage. It batches + /// those updates into one O(n) driver scan per dirty layer. /// \throws CoreError when the dataset is read-only or the index is invalid. void SetCellString(int fieldIndex, std::int64_t featureIndex, const std::string& value); void SetCellDouble(int fieldIndex, std::int64_t featureIndex, double value); @@ -151,9 +150,13 @@ namespace mwg [[nodiscard]] std::filesystem::path Path() const; [[nodiscard]] bool IsWritable() const; - /// Writes pending changes to disk without closing. Closing the dataset - /// does this as well. - /// \throws CoreError when GDAL cannot flush pending changes. + /// Writes pending changes to disk without closing and recomputes stored + /// Shapefile and GeoPackage bounds after geometry edits. + /// Closing alone writes features but does not guarantee exact stored + /// bounds. Other formats retain their driver's persistence behavior. + /// \throws CoreError when flushing or stored-bounds recomputation fails. + /// A failure does not roll back geometry writes. Recompute failures + /// leave the layer dirty so a later Flush() can retry. void Flush(); [[nodiscard]] int LayerCount() const; diff --git a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp index 9941984..6e236c6 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp @@ -59,7 +59,6 @@ namespace mwg struct PersistentOutput final { std::string path; - std::string driverName; std::vector openOptions; unsigned int openFlags = GDAL_OF_READONLY | GDAL_OF_VERBOSE_ERROR; }; @@ -88,7 +87,8 @@ namespace mwg PersistentOutput result; result.path = hasDescription ? description : outputName; - result.driverName = driverName != nullptr ? driverName : ""; + if (dataset->GetAccess() == GA_Update) + result.openFlags |= GDAL_OF_UPDATE; if (dataset->GetRasterCount() > 0) result.openFlags |= GDAL_OF_RASTER; if (dataset->GetLayerCount() > 0) @@ -123,10 +123,9 @@ namespace mwg GdalDatasetPtr ReopenPersistentOutput(const PersistentOutput& output) { - if (output.driverName.empty() || output.path.empty()) + if (output.path.empty() || output.path.starts_with("/vsistdout/")) return {}; - const char* const allowedDrivers[]{output.driverName.c_str(), nullptr}; std::vector openOptions; openOptions.reserve(output.openOptions.size() + 1); for (const std::string& option : output.openOptions) @@ -136,7 +135,7 @@ namespace mwg return GdalDatasetPtr(static_cast( GDALOpenEx(output.path.c_str(), output.openFlags, - allowedDrivers, + nullptr, output.openOptions.empty() ? nullptr : openOptions.data(), nullptr))); } @@ -191,7 +190,7 @@ namespace mwg { std::unique_ptr algorithm; GdalDatasetPtr outputDataset; - GDALDataset* callerProvidedOutputDataset = nullptr; + GdalDatasetPtr callerProvidedOutputDataset; std::vector errors; bool configurationValid = true; bool runAttempted = false; @@ -282,8 +281,11 @@ namespace mwg return false; } - return SetArgumentValue(_impl->algorithm.get(), _impl->errors, - _impl->configurationValid, name, std::string(value)); + const bool succeeded = SetArgumentValue(_impl->algorithm.get(), _impl->errors, + _impl->configurationValid, name, std::string(value)); + if (succeeded && name == "output") + _impl->callerProvidedOutputDataset.reset(); + return succeeded; } bool GdalAlgorithmRunner::SetArgument(const std::string_view name, const char* value) @@ -296,8 +298,7 @@ namespace mwg _impl->configurationValid = false; return false; } - return SetArgumentValue(_impl->algorithm.get(), _impl->errors, - _impl->configurationValid, name, std::string(value)); + return SetArgument(name, std::string_view(value)); } bool GdalAlgorithmRunner::SetArgument(const std::string_view name, const bool value) @@ -401,7 +402,8 @@ namespace mwg else if (argument->GetType() == GAAT_DATASET && argument->GetName() == "output") { - _impl->callerProvidedOutputDataset = value.get(); + _impl->callerProvidedOutputDataset.reset( + argument->Get().GetDatasetIncreaseRefCount()); } return succeeded; } @@ -425,6 +427,15 @@ namespace mwg GdalErrorScope scope; const bool succeeded = _impl->algorithm->ParseCommandLineArguments(args); + GDALAlgorithm* const actualAlgorithm = GetActualAlgorithm(_impl->algorithm.get()); + GDALAlgorithmArg* const output = actualAlgorithm != nullptr + ? actualAlgorithm->GetArg("output") : nullptr; + if (output == nullptr || output->GetType() != GAAT_DATASET || + output->Get().GetDatasetRef() != + _impl->callerProvidedOutputDataset.get()) + { + _impl->callerProvidedOutputDataset.reset(); + } const std::size_t previousCount = _impl->errors.size(); AppendErrors(_impl->errors, scope); scope.RethrowCaptureException(); @@ -511,7 +522,7 @@ namespace mwg persistentOutput = DescribePersistentOutput(*outputValue, outputDataset); const bool callerProvidedOutput = outputDataset != nullptr && - outputDataset == _impl->callerProvidedOutputDataset; + outputDataset == _impl->callerProvidedOutputDataset.get(); if (!persistentOutput || callerProvidedOutput) { _impl->outputDataset.reset( @@ -520,7 +531,8 @@ namespace mwg } finalized = Finalize(); } - if (_impl->outputDataset != nullptr) + if (finalized && !adapter.WasCancelled() && !HasFailure(_impl->errors) && + _impl->outputDataset != nullptr) { GdalErrorScope outputScope; const std::size_t outputErrorCount = _impl->errors.size(); @@ -535,7 +547,8 @@ namespace mwg "GDAL algorithm output could not be flushed."); } } - else if (succeeded && persistentOutput.has_value() && outputValue != nullptr) + else if (finalized && !adapter.WasCancelled() && !HasFailure(_impl->errors) && + persistentOutput.has_value() && outputValue != nullptr) { GdalErrorScope outputScope; const std::size_t outputErrorCount = _impl->errors.size(); @@ -549,18 +562,31 @@ namespace mwg outputErrorCount, "GDAL algorithm output could not be closed."); } - else + else if (!HasFailure(_impl->errors)) { GdalErrorScope reopenScope; _impl->outputDataset = ReopenPersistentOutput(*persistentOutput); - AppendErrors(_impl->errors, reopenScope); + if (_impl->outputDataset == nullptr && + (persistentOutput->openFlags & GDAL_OF_UPDATE) != 0) + { + persistentOutput->openFlags &= ~GDAL_OF_UPDATE; + _impl->outputDataset = ReopenPersistentOutput(*persistentOutput); + } + // Reopening is a convenience, not part of completing the write. + // Some creation drivers have no reader, or support only read-only access. + for (GdalError error : reopenScope.Errors()) + { + if (error.errorClass >= GdalErrorClass::Failure) + error.errorClass = GdalErrorClass::Warning; + _impl->errors.push_back(std::move(error)); + } reopenScope.RethrowCaptureException(); if (_impl->outputDataset == nullptr) { - AddError( - _impl->errors, - "GDAL algorithm output could not be reopened read-only: " + - persistentOutput->path); + _impl->errors.push_back({ + GdalErrorClass::Warning, kRunnerErrorCode, + "GDAL algorithm output was written but no dataset could be reopened: " + + persistentOutput->path}); } } } diff --git a/src/MapWinGIS.Core/src/gdal/GdalEnvironment.cpp b/src/MapWinGIS.Core/src/gdal/GdalEnvironment.cpp index daffae0..80ce1d9 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalEnvironment.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalEnvironment.cpp @@ -167,7 +167,10 @@ namespace mwg std::string GdalEnvironment::LastErrorMessage() { - return t_lastError; + if (!t_lastError.empty()) + return t_lastError; + const char* const message = CPLGetLastErrorMsg(); + return message != nullptr ? message : ""; } void GdalEnvironment::ClearLastError() diff --git a/src/MapWinGIS.Core/src/gdal/GdalRuntime.cpp b/src/MapWinGIS.Core/src/gdal/GdalRuntime.cpp index 79051c1..0719343 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalRuntime.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalRuntime.cpp @@ -15,6 +15,7 @@ #include "internal/GdalProgressAdapterAccess.h" #include +#include #include #include #include @@ -218,7 +219,8 @@ namespace mwg Callback callback; const GdalCancellationToken* cancellationToken; - bool cancelled = false; + std::atomic_bool cancelled{false}; + mutable std::mutex callbackMutex; std::exception_ptr callbackException; }; @@ -237,6 +239,7 @@ namespace mwg void GdalProgressAdapter::RethrowCallbackException() const { + const std::lock_guard lock(_impl->callbackMutex); if (_impl->callbackException) std::rethrow_exception(_impl->callbackException); } @@ -258,6 +261,7 @@ namespace mwg return 0; auto& impl = *static_cast(userData); + const std::lock_guard lock(impl.callbackMutex); if (impl.cancelled) return 0; if (impl.cancellationToken != nullptr && impl.cancellationToken->IsCancellationRequested()) diff --git a/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp b/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp index b1dd26b..fb65490 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp @@ -447,14 +447,6 @@ namespace mwg _impl->cancelled = false; if (HasNullByte(options)) return RecordFailure(_impl->errors, "GDAL utility options must not contain null bytes."); - if (progress && - std::find(options.begin(), options.end(), "-multi") != options.end()) - { - return RecordFailure( - _impl->errors, - "GDAL raster warp option '-multi' is incompatible with synchronous progress callbacks."); - } - return _impl->succeeded = RunUtilityOperation( _impl->errors, _impl->cancelled, std::move(progress), cancellationToken, [&](GdalProgressAdapter& adapter) diff --git a/src/MapWinGIS.Core/src/vector/VectorDataset.cpp b/src/MapWinGIS.Core/src/vector/VectorDataset.cpp index 6370ae1..0462e65 100644 --- a/src/MapWinGIS.Core/src/vector/VectorDataset.cpp +++ b/src/MapWinGIS.Core/src/vector/VectorDataset.cpp @@ -47,6 +47,7 @@ namespace mwg { CPLErrorReset(); OGRFeatureUniquePtr feature{layer.GetNextFeature()}; + GdalEnvironment::RethrowLogException(); if (feature == nullptr && CPLGetLastErrorType() >= CE_Failure) { std::string message = "Failed to read vector layer features"; @@ -193,7 +194,14 @@ namespace mwg void Save(OGRFeature* feature, const bool geometryChanged = false) const { GdalEnvironment::ClearLastError(); - if (_layer->SetFeature(feature) != OGRERR_NONE) + const OGRErr result = _layer->SetFeature(feature); + if (geometryChanged) + { + _extent.reset(); + _extentNeedsExactRecalculation = true; + _storedExtentDirty = true; + } + if (result != OGRERR_NONE) { const GIntBig featureId = feature->GetFID(); _cachedFeature.reset(); @@ -201,42 +209,47 @@ namespace mwg _featureIds.clear(); _featureIdsValid = false; _featureCount.reset(); - if (geometryChanged) - { - _extent.reset(); - _extentNeedsExactRecalculation = true; - } - + GdalEnvironment::RethrowLogException(); std::string message = "Cannot write feature " + std::to_string(featureId); - if (const std::string detail = CPLGetLastErrorMsg(); !detail.empty()) + if (const std::string detail = GdalEnvironment::LastErrorMessage(); !detail.empty()) message += " (" + detail + ")"; throw CoreError(message); } - if (geometryChanged) + GdalEnvironment::RethrowLogException(); + } + + void FlushStoredExtent() const + { + if (!_storedExtentDirty) + return; + const GDALDriver* const driver = _dataset.GetDriver(); + const char* const driverName = driver != nullptr ? driver->GetDescription() : nullptr; + if (driverName != nullptr && + (std::string_view(driverName) == "ESRI Shapefile" || + std::string_view(driverName) == "GPKG")) { - _extent.reset(); - _extentNeedsExactRecalculation = true; - if (const GDALDriver* driver = _dataset.GetDriver(); - driver != nullptr && - std::string_view(driver->GetDescription()) == "ESRI Shapefile") + const char* const layerName = _layer->GetName(); + if (layerName == nullptr || layerName[0] == '\0') + throw CoreError("Cannot recompute stored extent for an unnamed layer"); + + const std::string sql = "RECOMPUTE EXTENT ON " + std::string(layerName); + const auto releaseResult = [this](OGRLayer* result) { - // Shapefile SetFeature expands stored bounds but does not shrink them. - const std::string sql = - "RECOMPUTE EXTENT ON " + std::string(_layer->GetName()); - GdalEnvironment::ClearLastError(); - OGRLayer* result = _dataset.ExecuteSQL(sql.c_str(), nullptr, nullptr); - const CPLErr errorType = CPLGetLastErrorType(); - const std::string detail = CPLGetLastErrorMsg(); if (result != nullptr) _dataset.ReleaseResultSet(result); - if (errorType >= CE_Failure) - { - throw CoreError("Geometry was written, but cannot recompute stored " - "shapefile extent: " + detail); - } - _extentNeedsExactRecalculation = false; + }; + GdalEnvironment::ClearLastError(); + const std::unique_ptr result{ + _dataset.ExecuteSQL(sql.c_str(), nullptr, nullptr), releaseResult}; + const CPLErr errorType = CPLGetLastErrorType(); + GdalEnvironment::RethrowLogException(); + if (errorType >= CE_Failure) + { + throw CoreError("Geometry was written, but cannot recompute stored " + "vector layer extent: " + GdalEnvironment::LastErrorMessage()); } } + _storedExtentDirty = false; } [[nodiscard]] std::int64_t FeatureCount() const @@ -275,15 +288,17 @@ namespace mwg { GdalEnvironment::ClearLastError(); OGREnvelope envelope; - if (_layer->GetExtent(&envelope, TRUE) == OGRERR_NONE) + const OGRErr result = _layer->GetExtent(&envelope, TRUE); + GdalEnvironment::RethrowLogException(); + if (result == OGRERR_NONE) { _extent = Extent{envelope.MinX, envelope.MinY, envelope.MaxX, envelope.MaxY}; } else { - const std::string detail = CPLGetLastErrorMsg(); + const std::string detail = GdalEnvironment::LastErrorMessage(); const CPLErr errorType = CPLGetLastErrorType(); - if (errorType >= CE_Failure || !detail.empty()) + if (errorType >= CE_Failure) { std::string message = "Cannot determine vector layer extent"; if (!detail.empty()) @@ -317,7 +332,7 @@ namespace mwg // Keep expected diagnostics suppressed for the entire scan, not // just while SetIgnoredFields is configured. const GdalEnvironment::QuietScope quiet; - const IgnoreAllButIds ignoreScope(*_layer, _fields); + const IgnoredFields ignoreScope(*_layer, _fields, true); _featureIds.clear(); @@ -350,6 +365,7 @@ namespace mwg [[nodiscard]] Extent ComputeExactExtent() const { OGRLayer& layer = *_layer; + const IgnoredFields ignoreScope(layer, _fields, false); layer.ResetReading(); const ReadingReset readingReset{layer}; @@ -369,30 +385,31 @@ namespace mwg return extent.value_or(Extent{}); } - /// Tells the driver to skip attributes and geometry for the duration of - /// the id scan, and restores the layer afterwards. - class IgnoreAllButIds + /// Skips attributes and optionally geometry, then restores the layer. + class IgnoredFields { public: - IgnoreAllButIds(OGRLayer& layer, const std::vector& fields) + IgnoredFields(OGRLayer& layer, const std::vector& fields, + const bool ignoreGeometry) : _layer(layer) { std::vector ignored; ignored.reserve(fields.size() + 2); for (const FieldInfo& field : fields) ignored.push_back(field.name.c_str()); - ignored.push_back("OGR_GEOMETRY"); + if (ignoreGeometry) + ignored.push_back("OGR_GEOMETRY"); ignored.push_back(nullptr); _applied = _layer.SetIgnoredFields(ignored.data()) == OGRERR_NONE; } - ~IgnoreAllButIds() + ~IgnoredFields() { if (_applied) _layer.SetIgnoredFields(nullptr); } - IgnoreAllButIds(const IgnoreAllButIds&) = delete; - IgnoreAllButIds& operator=(const IgnoreAllButIds&) = delete; + IgnoredFields(const IgnoredFields&) = delete; + IgnoredFields& operator=(const IgnoredFields&) = delete; private: OGRLayer& _layer; @@ -406,6 +423,7 @@ namespace mwg mutable std::optional _featureCount; mutable std::optional _extent; mutable bool _extentNeedsExactRecalculation = false; + mutable bool _storedExtentDirty = false; }; std::int64_t detail::VectorLayerAccess::ForEachGeometry( @@ -700,12 +718,16 @@ namespace mwg void VectorDataset::Flush() { + for (const auto& layer : _impl->_layers) + layer->_impl->FlushStoredExtent(); GdalEnvironment::ClearLastError(); - if (_impl->_dataset->FlushCache(false) != CE_None) + const CPLErr result = _impl->_dataset->FlushCache(false); + GdalEnvironment::RethrowLogException(); + if (result != CE_None) { std::string message = "Cannot flush vector data source: " + ToUtf8(_impl->_path); - if (const char* detail = CPLGetLastErrorMsg(); detail != nullptr && *detail != '\0') - message += " (" + std::string(detail) + ")"; + if (const std::string detail = GdalEnvironment::LastErrorMessage(); !detail.empty()) + message += " (" + detail + ")"; throw CoreError(message); } } diff --git a/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp b/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp index aef875b..bff67ae 100644 --- a/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp +++ b/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp @@ -589,6 +589,32 @@ TEST(VectorLayerRenderer, GdiClipsCrossingPolygonsAndRestoresTheHostClip) } } +TEST(VectorLayerRenderer, GdiSkipsLayerTraversalWhenTheHostClipDoesNotIntersectBounds) +{ + BitmapSurface surface(160, 160); + ASSERT_TRUE(surface.IsValid()); + ASSERT_NE(IntersectClipRect(surface.Get(), 140, 140, 160, 160), ERROR); + RECT originalClip{}; + ASSERT_NE(GetClipBox(surface.Get(), &originalClip), ERROR); + const RECT bounds{0, 0, 120, 100}; + const mwg::Viewport viewport = FitViewport(bounds); + auto scene = MakeConvexPolygonScene(); + FirstPolygon(scene).front().front().x = std::numeric_limits::infinity(); + const int saved = SaveDC(surface.Get()); + ASSERT_NE(saved, 0); + ASSERT_NE(IntersectClipRect(surface.Get(), bounds.left, bounds.top, bounds.right, bounds.bottom), + ERROR); + RECT intersection{}; + EXPECT_EQ(GetClipBox(surface.Get(), &intersection), NULLREGION); + ASSERT_TRUE(RestoreDC(surface.Get(), saved)); + mwg::activex::GdiVectorLayerRenderer renderer; + EXPECT_EQ(renderer.Draw(surface.Get(), bounds, scene, viewport), S_OK); + EXPECT_TRUE(renderer.GetFailedLayers().empty()); + RECT restoredClip{}; + ASSERT_NE(GetClipBox(surface.Get(), &restoredClip), ERROR); + EXPECT_TRUE(EqualRect(&originalClip, &restoredClip)); +} + TEST(VectorLayerRenderer, GdiAndOpenGlAgreeAtFractionalRectangleEdges) { BitmapSurface gdiSurface(120, 100); diff --git a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp index b40cdae..67d6594 100644 --- a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp @@ -265,6 +265,8 @@ namespace static GDALDataset* outputDataset; static bool runSucceeded; static bool rejectOpen; + static inline bool finalizeSucceeded = true; + static inline int openCount = 0; OutputFailureAlgorithm() : GDALAlgorithm( @@ -275,8 +277,18 @@ namespace AddArg("output", 0, "Output dataset", &_output); } + bool Finalize() override + { + if (!finalizeSucceeded) + { + ReportError(CE_Failure, CPLE_AppDefined, "%s", "forced output finalization failure"); + return false; + } + return GDALAlgorithm::Finalize(); + } + protected: - bool RunImpl(GDALProgressFunc, void*) override + bool RunImpl(GDALProgressFunc progress, void* userData) override { if (outputDataset == nullptr) { @@ -288,6 +300,8 @@ namespace _output.Set(output); outputDataset = nullptr; GDALReleaseDataset(output); + if (progress != nullptr) + (void)progress(1.0, "complete", userData); return runSucceeded; } @@ -295,6 +309,30 @@ namespace GDALArgDatasetValue _output; }; + class WorkerProgressAlgorithm final : public GDALAlgorithm + { + public: + WorkerProgressAlgorithm() + : GDALAlgorithm("mwg-test-worker-progress", "Test worker progress", "") + { + } + + protected: + bool RunImpl(GDALProgressFunc progress, void* userData) override + { + std::array results{}; + const auto report = [&](const std::size_t index) + { + results[index] = progress(0.5, "worker", userData); + }; + std::thread first(report, 0); + std::thread second(report, 1); + first.join(); + second.join(); + return results[0] != 0 && results[1] != 0; + } + }; + class CallerOwnedOutputAlgorithm final : public GDALAlgorithm { public: @@ -345,6 +383,8 @@ namespace [] { return std::make_unique(); }); registry.DeclareAlgorithm({"mwg-test", "caller-owned-output"}, [] { return std::make_unique(); }); + registry.DeclareAlgorithm({"mwg-test", "worker-progress"}, + [] { return std::make_unique(); }); }); } @@ -639,6 +679,51 @@ TEST(GdalAlgorithmRunner, SerializesConcurrentRunnersAndIsolatesTheirResults) EXPECT_EQ(results[1].DetailedErrorMsg().find("first operation"), std::string::npos); } +TEST(GdalAlgorithmRunner, SerializesWorkerCallbacksAndRetainsCancellationAndExceptions) +{ + GdalEnvironment::Initialize(); + RegisterTestAlgorithms(); + int callbackCount = 0; + std::atomic_int active{0}; + std::atomic_bool overlapped{false}; + GdalAlgorithmRunner runner("mwg-test worker-progress"); + EXPECT_TRUE(runner.Run([&](double, std::string_view) + { + if (active.fetch_add(1) != 0) + overlapped.store(true); + ++callbackCount; + std::this_thread::sleep_for(std::chrono::milliseconds(5)); + active.fetch_sub(1); + return true; + })); + EXPECT_EQ(callbackCount, 2); + EXPECT_FALSE(overlapped.load()); + + GdalAlgorithmRunner cancelled("mwg-test worker-progress"); + callbackCount = 0; + EXPECT_FALSE(cancelled.Run([&](double, std::string_view) + { + ++callbackCount; + return false; + })); + EXPECT_EQ(callbackCount, 1); + EXPECT_TRUE(cancelled.Result().WasCancelled()); + + GdalAlgorithmRunner failed("mwg-test worker-progress"); + EXPECT_FALSE(failed.Run([](double, std::string_view) -> bool + { + throw std::runtime_error("worker callback failed"); + })); + EXPECT_FALSE(failed.Result().WasCancelled()); + EXPECT_NE(failed.Result().ErrorMsg().find("worker callback failed"), std::string::npos); + + GdalAlgorithmRunner reentered("mwg-test worker-progress"); + GdalAlgorithmRunner nested("mwg-test progress"); + EXPECT_FALSE(reentered.Run([&](double, std::string_view) { return nested.Run(); })); + EXPECT_NE(reentered.Result().ErrorMsg().find("cannot be called from a GDAL callback"), + std::string::npos); +} + TEST(GdalAlgorithmRunner, PreservesLoggerExceptionsUntilAfterFinalization) { GdalEnvironment::Initialize(); @@ -685,6 +770,7 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS { return nullptr; } + ++OutputFailureAlgorithm::openCount; if (OutputFailureAlgorithm::rejectOpen) { CPLError(CE_Failure, CPLE_OpenFailed, "%s", @@ -704,12 +790,16 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS bool runSucceeded; bool useGroupCommandLine; bool rejectReopen; + bool finalizeSucceeded; + bool cancel; }; for (const FailureCase& failure : { - FailureCase{"flush", "Simulated output flush failure", true, false, false}, - FailureCase{"close", "Simulated output close failure", true, false, false}, - FailureCase{"close", "Simulated output close failure", false, true, false}, - FailureCase{"reopen", "could not be reopened read-only", true, false, true}}) + FailureCase{"flush", "Simulated output flush failure", true, false, false, true, false}, + FailureCase{"close", "Simulated output close failure", true, false, false, true, false}, + FailureCase{"close", "Simulated output close failure", false, true, false, true, false}, + FailureCase{"reopen", "no dataset could be reopened", true, false, true, true, false}, + FailureCase{"finalize", "forced output finalization failure", true, false, true, false, false}, + FailureCase{"cancel", "cancelled", true, false, true, true, true}}) { const std::string path = std::string("MWG_OUTPUT_FAILURE:output:") + failure.kind; @@ -719,6 +809,8 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS OutputFailureAlgorithm::outputDataset = failingOutput.release(); OutputFailureAlgorithm::runSucceeded = failure.runSucceeded; OutputFailureAlgorithm::rejectOpen = failure.rejectReopen; + OutputFailureAlgorithm::finalizeSucceeded = failure.finalizeSucceeded; + OutputFailureAlgorithm::openCount = 0; GdalAlgorithmRunner failingRunner( failure.useGroupCommandLine ? "mwg-test" : "mwg-test output-failure"); if (failure.useGroupCommandLine) @@ -728,10 +820,15 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS })) << LastError(failingRunner); } - EXPECT_FALSE(failingRunner.Run()); + const bool successExpected = std::string_view(failure.kind) == "reopen"; + EXPECT_EQ(failingRunner.Run([&](double, std::string_view) { return !failure.cancel; }), + successExpected); + if (!failure.finalizeSucceeded || failure.cancel) + EXPECT_EQ(OutputFailureAlgorithm::openCount, 0); OutputFailureAlgorithm::rejectOpen = false; + OutputFailureAlgorithm::finalizeSucceeded = true; EXPECT_NE( - failingRunner.Result().ErrorMsg().find(failure.diagnostic), + failingRunner.Result().DetailedErrorMsg().find(failure.diagnostic), std::string::npos) << failingRunner.Result().DetailedErrorMsg(); OutputFailureAlgorithm::outputDataset = nullptr; @@ -754,6 +851,79 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS EXPECT_EQ(saved->GetRasterYSize(), 24); } +TEST(GdalAlgorithmRunner, ReopensWritableOutputsWithUpdateAccess) +{ + GdalEnvironment::Initialize(); + const test::TempDirectory directory; + const auto input = test::MakeRaster(directory.Path()); + const auto path = directory.Path() / "editable.tif"; + GdalDatasetPtr output; + { + GdalAlgorithmRunner runner("raster convert"); + ASSERT_TRUE(runner.SetArgument("input", ToUtf8(input))); + ASSERT_TRUE(runner.SetArgument("output", ToUtf8(path))); + ASSERT_TRUE(runner.Run()) << LastError(runner); + output = runner.TakeOutputDataset(); + } + ASSERT_NE(output, nullptr); + EXPECT_EQ(output->GetAccess(), GA_Update); + EXPECT_EQ(output->GetRasterBand(1)->SetNoDataValue(255), CE_None); + EXPECT_EQ(output->SetMetadataItem("MWG_TEST", "edited"), CE_None); + int overviewLevel = 2; + EXPECT_EQ(output->BuildOverviews("NEAREST", 1, &overviewLevel, 0, nullptr, nullptr, nullptr), + CE_None); + ASSERT_EQ(GDALClose(output.release()), CE_None); + GdalDatasetPtr reopened(static_cast(GDALOpen(ToUtf8(path).c_str(), GA_ReadOnly))); + ASSERT_NE(reopened, nullptr); + EXPECT_STREQ(reopened->GetMetadataItem("MWG_TEST"), "edited"); + EXPECT_DOUBLE_EQ(reopened->GetRasterBand(1)->GetNoDataValue(), 255); + EXPECT_EQ(reopened->GetRasterBand(1)->GetOverviewCount(), 1); +} + +TEST(GdalAlgorithmRunner, ReopensAnOutputWithADifferentReader) +{ + GdalEnvironment::Initialize(); + RegisterTestAlgorithms(); + const test::TempDirectory directory; + const auto path = test::MakeRaster(directory.Path()); + auto writer = std::make_unique(); + writer->SetDescription("MWGWriteOnlyTest"); + // Use a synthetic writer handle for an existing TIFF. Only GTiff can reopen it. + class WriterDataset final : public GDALDataset + { + public: + WriterDataset(GDALDriver& driver, const std::string& name) + { + poDriver = &driver; + SetDescription(name.c_str()); + } + }; + GdalDatasetPtr output(std::make_unique(*writer, ToUtf8(path)).release()); + OutputFailureAlgorithm::outputDataset = output.release(); + OutputFailureAlgorithm::runSucceeded = true; + GdalAlgorithmRunner runner("mwg-test output-failure"); + ASSERT_TRUE(runner.Run()) << LastError(runner); + auto reopened = runner.TakeOutputDataset(); + ASSERT_NE(reopened, nullptr); + EXPECT_STREQ(reopened->GetDriverName(), "GTiff"); + EXPECT_EQ(reopened->GetRasterXSize(), 12); +} + +TEST(GdalAlgorithmRunner, CompletesWriteOnlyStreamingOutputsWithoutReopening) +{ + GdalEnvironment::Initialize(); + RegisterTestAlgorithms(); + auto output = std::make_unique(false, false); + output->SetDescription("/vsistdout/"); + OutputFailureAlgorithm::outputDataset = output.release(); + OutputFailureAlgorithm::runSucceeded = true; + GdalAlgorithmRunner runner("mwg-test output-failure"); + EXPECT_TRUE(runner.Run()) << LastError(runner); + EXPECT_EQ(runner.TakeOutputDataset(), nullptr); + EXPECT_NE(runner.Result().DetailedErrorMsg().find("no dataset could be reopened"), + std::string::npos); +} + TEST(GdalAlgorithmRunner, RejectsPreCancelledRunBeforeInvokingAlgorithm) { GdalEnvironment::Initialize(); diff --git a/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp b/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp index 1006e6e..afa800b 100644 --- a/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp @@ -23,6 +23,12 @@ using namespace mwg; namespace { + std::string ToUtf8(const std::filesystem::path& path) + { + const std::u8string utf8 = path.u8string(); + return {reinterpret_cast(utf8.data()), utf8.size()}; + } + // Lower bounds, not exact counts: the driver set depends on the vcpkg // manifest feature that was built (core-drivers or full-drivers). constexpr int kMinimumRasterDrivers = 50; @@ -78,9 +84,9 @@ TEST(GdalEnvironment, DeploysRuntimeDataNextToTheModule) ASSERT_FALSE(dir.empty()); EXPECT_TRUE(std::filesystem::is_directory(dir / "gdal-data")) - << "gdal-data is missing from " << dir.string(); + << "gdal-data is missing from " << ToUtf8(dir); EXPECT_TRUE(std::filesystem::exists(dir / "proj" / "proj.db")) - << "proj.db is missing from " << dir.string(); + << "proj.db is missing from " << ToUtf8(dir); } #ifdef MWG_TEST_FULL_DRIVERS @@ -94,7 +100,7 @@ TEST(GdalEnvironment, IncludesTheReleaseDriverSet) GDALDriver* const sqlite = GetGDALDriverManager()->GetDriverByName("SQLite"); ASSERT_NE(sqlite, nullptr); const test::TempDirectory temp; - const std::string path = (temp.Path() / "spatialite.sqlite").string(); + const std::string path = ToUtf8(temp.Path() / "spatialite.sqlite"); char spatialiteOption[] = "SPATIALITE=YES"; char* options[] = {spatialiteOption, nullptr}; GDALDataset* const dataset = sqlite->Create( diff --git a/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp b/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp index f76fa4d..b7dd16f 100644 --- a/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp @@ -263,35 +263,30 @@ TEST(GdalUtils, WarpsRasterUsingLegacyOptions) EXPECT_STREQ(dataset->GetSpatialRef()->GetAuthorityCode(nullptr), "3857"); } -TEST(GdalUtils, RejectsMultiWarpWhenSynchronousProgressCallbackIsProvided) +TEST(GdalUtils, SupportsThreadedWarpOptionsWithProgress) { GdalEnvironment::Initialize(); GdalEnvironment::QuietScope quiet; test::TempDirectory directory; const std::filesystem::path input = test::MakeRaster(directory.Path()); - const std::filesystem::path rejectedOutput = directory.Path() / "rejected.tif"; - const std::filesystem::path noCallbackOutput = directory.Path() / "multi.tif"; - const std::thread::id callerThread = std::this_thread::get_id(); - std::atomic_int callbackCount = 0; - std::atomic_bool callbackRanOffThread = false; GdalUtils utils; - - EXPECT_FALSE(utils.GdalRasterWarp( - input, rejectedOutput, {"-multi"}, - [&](double, std::string_view) - { - ++callbackCount; - callbackRanOffThread.store(std::this_thread::get_id() != callerThread); - return true; - })); - EXPECT_EQ(callbackCount.load(), 0); - EXPECT_FALSE(callbackRanOffThread.load()); - EXPECT_NE(utils.Result().ErrorMsg().find("'-multi'"), std::string::npos); - EXPECT_NE(utils.Result().ErrorMsg().find("synchronous"), std::string::npos); - EXPECT_FALSE(std::filesystem::exists(rejectedOutput)); - - EXPECT_TRUE(utils.GdalRasterWarp(input, noCallbackOutput, {"-multi"}, {}, nullptr)) - << utils.DetailedErrorMsg(); + for (const auto& options : { + std::vector{"-multi"}, + std::vector{"-wo", "NUM_THREADS=2"}, + std::vector{"-multi", "-wo", "NUM_THREADS=2"}}) + { + const auto output = directory.Path() / "threaded.tif"; + std::atomic_int callbackCount{0}; + EXPECT_TRUE(utils.GdalRasterWarp(input, output, options, + [&](double, std::string_view) + { + ++callbackCount; + return true; + })) << utils.DetailedErrorMsg(); + EXPECT_GT(callbackCount.load(), 0); + EXPECT_TRUE(std::filesystem::exists(output)); + std::filesystem::remove(output); + } } TEST(GdalUtils, TranslatesVectorAndPreservesFeatures) diff --git a/tests/MapWinGIS.Core.Tests/TestData.cpp b/tests/MapWinGIS.Core.Tests/TestData.cpp index 590ccd4..6050a5f 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.cpp +++ b/tests/MapWinGIS.Core.Tests/TestData.cpp @@ -84,7 +84,7 @@ namespace mwg::test feature->SetFID(id); feature->SetField(0, _state.storedName.c_str()); feature->SetField(1, _state.storedValue); - if (!_state.ignoredFields && id != 1) + if (!_state.ignoredGeometry && id != 1) { OGRPolygon polygon; if (id == 0 && !_state.onlyEmptyGeometries) @@ -114,6 +114,12 @@ namespace mwg::test OGRErr SetIgnoredFields(CSLConstList fields) override { _state.ignoredFields = fields != nullptr; + _state.ignoredGeometry = false; + for (const char* const* field = fields; field != nullptr && *field != nullptr; ++field) + { + if (std::string_view(*field) == "OGR_GEOMETRY") + _state.ignoredGeometry = true; + } return OGRERR_NONE; } @@ -153,6 +159,11 @@ namespace mwg::test "Simulated extent failure"); return OGRERR_FAILURE; } + if (_state.warningExtentFailure) + { + CPLError(CE_Warning, CPLE_NotSupported, "Simulated unavailable extent"); + return OGRERR_FAILURE; + } if (_state.silentExtentFailure || _state.emptyLayer || _state.onlyEmptyGeometries) return OGRERR_FAILURE; @@ -172,8 +183,10 @@ namespace mwg::test { public: explicit VectorReadTestDataset(VectorReadTestDriver& state) - : _layer(state) + : _state(state), _layer(state) { + if (state.simulateShapefile) + poDriver = GetGDALDriverManager()->GetDriverByName("ESRI Shapefile"); } int GetLayerCount() const override @@ -186,7 +199,18 @@ namespace mwg::test return index == 0 ? &_layer : nullptr; } + OGRLayer* ExecuteSQL(const char* statement, OGRGeometry*, const char*) override + { + if (!std::string_view(statement).starts_with("RECOMPUTE EXTENT ON ")) + throw std::runtime_error("Unexpected test SQL statement"); + ++_state.recomputeExtentCalls; + if (_state.failRecomputeExtent) + CPLError(CE_Failure, CPLE_AppDefined, "Simulated extent recompute failure"); + return nullptr; + } + private: + VectorReadTestDriver& _state; VectorReadTestLayer _layer; }; } diff --git a/tests/MapWinGIS.Core.Tests/TestData.h b/tests/MapWinGIS.Core.Tests/TestData.h index cf3fe2f..7516cc2 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.h +++ b/tests/MapWinGIS.Core.Tests/TestData.h @@ -37,17 +37,22 @@ namespace mwg::test bool failOnSecondRead = false; bool failExtent = false; bool silentExtentFailure = false; + bool warningExtentFailure = false; bool failSetFeature = false; + bool simulateShapefile = false; + bool failRecomputeExtent = false; bool emptyLayer = false; bool onlyEmptyGeometries = false; bool unknownCount = false; bool ignoredFields = false; + bool ignoredGeometry = false; std::string storedName = "Original"; int storedValue = 0; int nextFeature = 0; int readCalls = 0; int countCalls = 0; int extentCalls = 0; + int recomputeExtentCalls = 0; private: std::unique_ptr _driver; diff --git a/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp b/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp index a952e38..3694d38 100644 --- a/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp @@ -12,6 +12,7 @@ #include #include +#include #include "TestData.h" #include "mapwingis/core/Error.h" @@ -93,10 +94,11 @@ TEST(VectorLayer, CountsFeaturesWhenTheDriverReportsAnUnknownCount) const VectorLayer& layer = dataset->Layer(0); EXPECT_EQ(layer.FeatureCount(), 3); - EXPECT_EQ(driver.readCalls, 4); + const int readsAfterScan = driver.readCalls; + EXPECT_GT(readsAfterScan, 0); EXPECT_EQ(layer.FeatureCount(), 3); EXPECT_EQ(layer.GeometryWkt(2), "POLYGON EMPTY"); - EXPECT_EQ(driver.readCalls, 4); + EXPECT_EQ(driver.readCalls, readsAfterScan); } TEST(VectorLayer, DoesNotCacheAnIncompleteFallbackFeatureCount) @@ -182,8 +184,10 @@ TEST(VectorLayer, ReportsAnEmptyExtentForNullAndEmptyGeometries) EXPECT_EQ(layer.FeatureCount(), 3); EXPECT_TRUE(layer.GetExtent().IsEmpty()); + const int readsAfterScan = driver.readCalls; + EXPECT_GT(readsAfterScan, 0); EXPECT_TRUE(layer.GetExtent().IsEmpty()); - EXPECT_EQ(driver.readCalls, 4); + EXPECT_EQ(driver.readCalls, readsAfterScan); EXPECT_EQ(driver.extentCalls, 1); } @@ -203,6 +207,17 @@ TEST(VectorLayer, RetriesTheFallbackExtentScanAfterAReadFailure) EXPECT_EQ(driver.extentCalls, 2); } +TEST(VectorLayer, FallsBackToGeometryScanWhenExtentIsUnavailableWithAWarning) +{ + VectorReadTestDriver driver; + driver.warningExtentFailure = true; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath); + const VectorLayer& layer = dataset->Layer(0); + EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); + EXPECT_GT(driver.readCalls, 0); + EXPECT_FALSE(driver.ignoredFields); +} + TEST_P(VectorLayerTest, ReportsTheExtent) { const Extent extent = _layer->GetExtent(); @@ -338,6 +353,7 @@ namespace std::unique_ptr dataset = VectorDataset::Open(_path, OpenMode::Update); ASSERT_TRUE(dataset->IsWritable()); edit(dataset->Layer(0)); + dataset->Flush(); } [[nodiscard]] std::unique_ptr Reopen() const @@ -541,6 +557,69 @@ TEST(VectorLayer, AttributeEditsDoNotInvalidateTheExtent) EXPECT_EQ(driver.extentCalls, 1); } +TEST(VectorLayer, FailedGeometryWritesDiscardTheEditedFeatureAndExtentCache) +{ + VectorReadTestDriver driver; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); + VectorLayer& layer = dataset->Layer(0); + ASSERT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); + const std::string original = layer.GeometryWkt(0); + driver.failSetFeature = true; + EXPECT_THROW(layer.SetGeometryWkt( + 0, "POLYGON ((10 0, 11 0, 11 1, 10 1, 10 0))"), CoreError); + EXPECT_EQ(layer.GeometryWkt(0), original); + EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); +} + +TEST(VectorLayer, BatchesStoredExtentUpdatesAndRetriesAfterRecomputeFailure) +{ + VectorReadTestDriver driver; + driver.simulateShapefile = true; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); + ASSERT_EQ(dataset->DriverName(), "ESRI Shapefile"); + VectorLayer& layer = dataset->Layer(0); + layer.SetGeometryWkt(0, "POLYGON EMPTY"); + layer.SetGeometryWkt(2, "POLYGON EMPTY"); + EXPECT_EQ(driver.recomputeExtentCalls, 0); + + driver.failRecomputeExtent = true; + try + { + dataset->Flush(); + FAIL() << "Expected stored extent recomputation to fail"; + } + catch (const CoreError& error) + { + EXPECT_NE(std::string(error.what()).find("Simulated extent recompute failure"), + std::string::npos); + } + EXPECT_EQ(driver.recomputeExtentCalls, 1); + driver.failRecomputeExtent = false; + EXPECT_NO_THROW(dataset->Flush()); + EXPECT_EQ(driver.recomputeExtentCalls, 2); + EXPECT_NO_THROW(dataset->Flush()); + EXPECT_EQ(driver.recomputeExtentCalls, 2); +} + +TEST(VectorLayer, PropagatesLoggerExceptionsFromWritesAndExtentQueries) +{ + VectorReadTestDriver driver; + const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); + VectorLayer& layer = dataset->Layer(0); + driver.failExtent = true; + GdalEnvironment::SetLogHandler([](LogLevel, std::string_view) + { + throw std::runtime_error("vector logger failure"); + }); + EXPECT_THROW((void)layer.GetExtent(), std::runtime_error); + driver.failSetFeature = true; + EXPECT_THROW(layer.SetCellString(layer.FieldIndex("NAME"), 0, "Rejected"), std::runtime_error); + GdalEnvironment::SetLogHandler({}); + EXPECT_EQ(layer.CellString(layer.FieldIndex("NAME"), 0), "Original"); + driver.failExtent = false; + EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); +} + TEST_P(VectorLayerEditTest, RecomputesTheExtentAfterAGeometryEdit) { static constexpr const char* kMoved = @@ -567,18 +646,17 @@ TEST_P(VectorLayerEditTest, ShrinksTheExtentAfterAGeometryEdit) EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 103000.0); } -TEST(VectorLayer, PersistsShrinkingShapefileBoundsWithoutAnExtentQuery) +TEST_P(VectorLayerEditTest, FlushPersistsShrinkingBoundsWithoutAnExtentQuery) { - const TempDirectory dir; - const auto path = MakeProvincesDataset(dir.Path(), "provinces.shp"); { - const auto dataset = VectorDataset::Open(path, OpenMode::Update); + const auto dataset = VectorDataset::Open(_path, OpenMode::Update); dataset->Layer(0).SetGeometryWkt( 3, "POLYGON ((100500 400000, 101500 400000, 101500 401000, " "100500 401000, 100500 400000))"); + dataset->Flush(); } - const auto reopened = VectorDataset::Open(path); + const auto reopened = Reopen(); EXPECT_DOUBLE_EQ(reopened->Layer(0).GetExtent().xMax, 103000.0); } From f62a1839688f7dacb9d7cd8418f40691948711b3 Mon Sep 17 00:00:00 2001 From: Paul Meems Date: Fri, 6 Nov 2026 22:33:03 +0100 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=90=9B=20fix(core):=20address=20verif?= =?UTF-8?q?ied=20PR=20feedback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Keep stored extent dirty state tied to successful geometry writes - Preserve GDAL raw layer-name syntax for extent recomputation - Avoid closing an already closed algorithm output Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/gdal/GdalAlgorithmRunner.cpp | 4 ++- .../src/vector/VectorDataset.cpp | 3 +- .../GdalAlgorithmRunnerTests.cpp | 31 +++++++++++++++---- tests/MapWinGIS.Core.Tests/TestData.cpp | 3 +- tests/MapWinGIS.Core.Tests/TestData.h | 2 ++ .../MapWinGIS.Core.Tests/VectorLayerTests.cpp | 18 +++++++++++ 6 files changed, 52 insertions(+), 9 deletions(-) diff --git a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp index 6e236c6..9ca1226 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp @@ -509,6 +509,7 @@ namespace mwg bool finalized = false; std::optional persistentOutput; GDALArgDatasetValue* outputValue = nullptr; + bool outputWasClosed = false; if (succeeded) { GDALAlgorithm* const actualAlgorithm = GetActualAlgorithm(_impl->algorithm.get()); @@ -553,6 +554,7 @@ namespace mwg GdalErrorScope outputScope; const std::size_t outputErrorCount = _impl->errors.size(); const bool outputClosed = outputValue->Close(); + outputWasClosed = outputClosed; AppendErrors(_impl->errors, outputScope); outputScope.RethrowCaptureException(); if (!outputClosed) @@ -631,7 +633,7 @@ namespace mwg ? actualAlgorithm->GetArg("output") : nullptr; bool outputClosed = true; - if (output != nullptr && output->GetType() == GAAT_DATASET) + if (output != nullptr && output->GetType() == GAAT_DATASET && !outputWasClosed) { outputClosed = output->Get().Close(); } diff --git a/src/MapWinGIS.Core/src/vector/VectorDataset.cpp b/src/MapWinGIS.Core/src/vector/VectorDataset.cpp index 0462e65..e538a55 100644 --- a/src/MapWinGIS.Core/src/vector/VectorDataset.cpp +++ b/src/MapWinGIS.Core/src/vector/VectorDataset.cpp @@ -199,7 +199,6 @@ namespace mwg { _extent.reset(); _extentNeedsExactRecalculation = true; - _storedExtentDirty = true; } if (result != OGRERR_NONE) { @@ -215,6 +214,8 @@ namespace mwg message += " (" + detail + ")"; throw CoreError(message); } + if (geometryChanged) + _storedExtentDirty = true; GdalEnvironment::RethrowLogException(); } diff --git a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp index 67d6594..7f3419a 100644 --- a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp @@ -223,9 +223,13 @@ namespace class FailingOutputDataset final : public GDALDataset { public: - FailingOutputDataset(const bool failFlush, const bool failClose) + static inline int closeCount = 0; + + FailingOutputDataset( + const bool failFlush, const bool failClose, const bool reportCloseDiagnostic = false) : _failFlush(failFlush), - _failClose(failClose) + _failClose(failClose), + _reportCloseDiagnostic(reportCloseDiagnostic) { } @@ -236,10 +240,17 @@ namespace CPLErr Close() override { + ++closeCount; const CPLErr flushResult = FlushCache(false); - return flushResult == CE_None - ? ReportFailure(_failClose, "Simulated output close failure") - : flushResult; + if (flushResult != CE_None) + return flushResult; + if (_reportCloseDiagnostic) + { + CPLError(CE_Failure, CPLE_FileIO, "%s", + "Simulated diagnostic during successful output close"); + return CE_None; + } + return ReportFailure(_failClose, "Simulated output close failure"); } private: @@ -257,6 +268,7 @@ namespace bool _failFlush; bool _failClose; + bool _reportCloseDiagnostic; }; class OutputFailureAlgorithm final : public GDALAlgorithm @@ -779,7 +791,8 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS } const bool failFlush = path.ends_with(":flush"); const bool failClose = path.ends_with(":close"); - return new FailingOutputDataset(failFlush, failClose); + const bool reportCloseDiagnostic = path.ends_with(":close-diagnostic"); + return new FailingOutputDataset(failFlush, failClose, reportCloseDiagnostic); }; GetGDALDriverManager()->RegisterDriver(driver.get()); @@ -796,6 +809,9 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS for (const FailureCase& failure : { FailureCase{"flush", "Simulated output flush failure", true, false, false, true, false}, FailureCase{"close", "Simulated output close failure", true, false, false, true, false}, + FailureCase{"close-diagnostic", + "Simulated diagnostic during successful output close", + true, false, false, true, false}, FailureCase{"close", "Simulated output close failure", false, true, false, true, false}, FailureCase{"reopen", "no dataset could be reopened", true, false, true, true, false}, FailureCase{"finalize", "forced output finalization failure", true, false, true, false, false}, @@ -811,6 +827,7 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS OutputFailureAlgorithm::rejectOpen = failure.rejectReopen; OutputFailureAlgorithm::finalizeSucceeded = failure.finalizeSucceeded; OutputFailureAlgorithm::openCount = 0; + FailingOutputDataset::closeCount = 0; GdalAlgorithmRunner failingRunner( failure.useGroupCommandLine ? "mwg-test" : "mwg-test output-failure"); if (failure.useGroupCommandLine) @@ -831,6 +848,8 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS failingRunner.Result().DetailedErrorMsg().find(failure.diagnostic), std::string::npos) << failingRunner.Result().DetailedErrorMsg(); + if (std::string_view(failure.kind) == "close-diagnostic") + EXPECT_EQ(FailingOutputDataset::closeCount, 1); OutputFailureAlgorithm::outputDataset = nullptr; } diff --git a/tests/MapWinGIS.Core.Tests/TestData.cpp b/tests/MapWinGIS.Core.Tests/TestData.cpp index 6050a5f..0951a9c 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.cpp +++ b/tests/MapWinGIS.Core.Tests/TestData.cpp @@ -42,7 +42,7 @@ namespace mwg::test public: explicit VectorReadTestLayer(VectorReadTestDriver& state) : _state(state), - _definition(OGRFeatureDefn::CreateFeatureDefn("features")) + _definition(OGRFeatureDefn::CreateFeatureDefn(state.layerName.c_str())) { _definition->Reference(); _definition->SetGeomType(wkbPolygon); @@ -201,6 +201,7 @@ namespace mwg::test OGRLayer* ExecuteSQL(const char* statement, OGRGeometry*, const char*) override { + _state.recomputeExtentStatement = statement; if (!std::string_view(statement).starts_with("RECOMPUTE EXTENT ON ")) throw std::runtime_error("Unexpected test SQL statement"); ++_state.recomputeExtentCalls; diff --git a/tests/MapWinGIS.Core.Tests/TestData.h b/tests/MapWinGIS.Core.Tests/TestData.h index 7516cc2..c1c6eb2 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.h +++ b/tests/MapWinGIS.Core.Tests/TestData.h @@ -46,6 +46,7 @@ namespace mwg::test bool unknownCount = false; bool ignoredFields = false; bool ignoredGeometry = false; + std::string layerName = "features"; std::string storedName = "Original"; int storedValue = 0; int nextFeature = 0; @@ -53,6 +54,7 @@ namespace mwg::test int countCalls = 0; int extentCalls = 0; int recomputeExtentCalls = 0; + std::string recomputeExtentStatement; private: std::unique_ptr _driver; diff --git a/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp b/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp index 3694d38..3f32445 100644 --- a/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp @@ -560,6 +560,7 @@ TEST(VectorLayer, AttributeEditsDoNotInvalidateTheExtent) TEST(VectorLayer, FailedGeometryWritesDiscardTheEditedFeatureAndExtentCache) { VectorReadTestDriver driver; + driver.simulateShapefile = true; const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); VectorLayer& layer = dataset->Layer(0); ASSERT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); @@ -569,12 +570,16 @@ TEST(VectorLayer, FailedGeometryWritesDiscardTheEditedFeatureAndExtentCache) 0, "POLYGON ((10 0, 11 0, 11 1, 10 1, 10 0))"), CoreError); EXPECT_EQ(layer.GeometryWkt(0), original); EXPECT_DOUBLE_EQ(layer.GetExtent().xMax, 1.0); + driver.failSetFeature = false; + EXPECT_NO_THROW(dataset->Flush()); + EXPECT_EQ(driver.recomputeExtentCalls, 0); } TEST(VectorLayer, BatchesStoredExtentUpdatesAndRetriesAfterRecomputeFailure) { VectorReadTestDriver driver; driver.simulateShapefile = true; + driver.layerName = "road \"section\""; const auto dataset = VectorDataset::Open(VectorReadTestDriver::kPath, OpenMode::Update); ASSERT_EQ(dataset->DriverName(), "ESRI Shapefile"); VectorLayer& layer = dataset->Layer(0); @@ -594,6 +599,7 @@ TEST(VectorLayer, BatchesStoredExtentUpdatesAndRetriesAfterRecomputeFailure) std::string::npos); } EXPECT_EQ(driver.recomputeExtentCalls, 1); + EXPECT_EQ(driver.recomputeExtentStatement, "RECOMPUTE EXTENT ON road \"section\""); driver.failRecomputeExtent = false; EXPECT_NO_THROW(dataset->Flush()); EXPECT_EQ(driver.recomputeExtentCalls, 2); @@ -601,6 +607,18 @@ TEST(VectorLayer, BatchesStoredExtentUpdatesAndRetriesAfterRecomputeFailure) EXPECT_EQ(driver.recomputeExtentCalls, 2); } +TEST(VectorLayer, FlushRecomputesExtentForShapefileLayerNamesWithSpaces) +{ + const TempDirectory directory; + const auto path = MakeProvincesDataset(directory.Path(), "road section.shp"); + const auto dataset = VectorDataset::Open(path, OpenMode::Update); + VectorLayer& layer = dataset->Layer(0); + + layer.SetGeometryWkt(0, "POLYGON ((100000 400000, 100001 400000, " + "100001 400001, 100000 400001, 100000 400000))"); + EXPECT_NO_THROW(dataset->Flush()); +} + TEST(VectorLayer, PropagatesLoggerExceptionsFromWritesAndExtentQueries) { VectorReadTestDriver driver; From b3b052acec6b591452da4566d032c33fcaadacc6 Mon Sep 17 00:00:00 2001 From: Paul Meems Date: Fri, 6 Nov 2026 22:34:21 +0100 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=90=9B=20fix(gdal):=20prefer=20config?= =?UTF-8?q?ured=20output=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use the explicit algorithm output name before a dataset description - Verify output reopening follows the configured name when descriptions differ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/gdal/GdalAlgorithmRunner.cpp | 2 +- .../GdalAlgorithmRunnerTests.cpp | 23 +++++++++++++++---- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp index 9ca1226..faf016d 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalAlgorithmRunner.cpp @@ -86,7 +86,7 @@ namespace mwg return std::nullopt; PersistentOutput result; - result.path = hasDescription ? description : outputName; + result.path = hasOutputName ? outputName : description; if (dataset->GetAccess() == GA_Update) result.openFlags |= GDAL_OF_UPDATE; if (dataset->GetRasterCount() > 0) diff --git a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp index 7f3419a..28b1b32 100644 --- a/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalAlgorithmRunnerTests.cpp @@ -279,6 +279,7 @@ namespace static bool rejectOpen; static inline bool finalizeSucceeded = true; static inline int openCount = 0; + static inline std::string lastOpenedPath; OutputFailureAlgorithm() : GDALAlgorithm( @@ -783,6 +784,7 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS return nullptr; } ++OutputFailureAlgorithm::openCount; + OutputFailureAlgorithm::lastOpenedPath = path; if (OutputFailureAlgorithm::rejectOpen) { CPLError(CE_Failure, CPLE_OpenFailed, "%s", @@ -814,6 +816,7 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS true, false, false, true, false}, FailureCase{"close", "Simulated output close failure", false, true, false, true, false}, FailureCase{"reopen", "no dataset could be reopened", true, false, true, true, false}, + FailureCase{"named-output", "", true, true, false, true, false}, FailureCase{"finalize", "forced output finalization failure", true, false, true, false, false}, FailureCase{"cancel", "cancelled", true, false, true, true, true}}) { @@ -822,11 +825,14 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS GdalDatasetPtr failingOutput(static_cast( GDALOpen(path.c_str(), GA_Update))); ASSERT_NE(failingOutput, nullptr); + if (std::string_view(failure.kind) == "named-output") + failingOutput->SetDescription("driver-specific output"); OutputFailureAlgorithm::outputDataset = failingOutput.release(); OutputFailureAlgorithm::runSucceeded = failure.runSucceeded; OutputFailureAlgorithm::rejectOpen = failure.rejectReopen; OutputFailureAlgorithm::finalizeSucceeded = failure.finalizeSucceeded; OutputFailureAlgorithm::openCount = 0; + OutputFailureAlgorithm::lastOpenedPath.clear(); FailingOutputDataset::closeCount = 0; GdalAlgorithmRunner failingRunner( failure.useGroupCommandLine ? "mwg-test" : "mwg-test output-failure"); @@ -837,17 +843,24 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS })) << LastError(failingRunner); } - const bool successExpected = std::string_view(failure.kind) == "reopen"; + const bool successExpected = + std::string_view(failure.kind) == "reopen" || + std::string_view(failure.kind) == "named-output"; EXPECT_EQ(failingRunner.Run([&](double, std::string_view) { return !failure.cancel; }), successExpected); if (!failure.finalizeSucceeded || failure.cancel) EXPECT_EQ(OutputFailureAlgorithm::openCount, 0); OutputFailureAlgorithm::rejectOpen = false; OutputFailureAlgorithm::finalizeSucceeded = true; - EXPECT_NE( - failingRunner.Result().DetailedErrorMsg().find(failure.diagnostic), - std::string::npos) - << failingRunner.Result().DetailedErrorMsg(); + if (failure.diagnostic[0] != '\0') + { + EXPECT_NE( + failingRunner.Result().DetailedErrorMsg().find(failure.diagnostic), + std::string::npos) + << failingRunner.Result().DetailedErrorMsg(); + } + if (std::string_view(failure.kind) == "named-output") + EXPECT_EQ(OutputFailureAlgorithm::lastOpenedPath, path); if (std::string_view(failure.kind) == "close-diagnostic") EXPECT_EQ(FailingOutputDataset::closeCount, 1); OutputFailureAlgorithm::outputDataset = nullptr;