diff --git a/.ai/memory/architecture/core-library.md b/.ai/memory/architecture/core-library.md index 9598c33..c9d067e 100644 --- a/.ai/memory/architecture/core-library.md +++ b/.ai/memory/architecture/core-library.md @@ -26,6 +26,15 @@ 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 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. + 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/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 24193e5..846912e 100644 --- a/.ai/memory/lessons/lessons.md +++ b/.ai/memory/lessons/lessons.md @@ -50,3 +50,17 @@ 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` 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 +[VectorLayerTests.cpp](../../../tests/MapWinGIS.Core.Tests/VectorLayerTests.cpp). diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 954c11f..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' @@ -46,14 +47,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..fb3d556 100644 --- a/Directory.Build.targets +++ b/Directory.Build.targets @@ -6,11 +6,12 @@ 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. --> - true + false +[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 -Force | Out-Null + Copy-Item "$repoRoot\Directory.Build.props", "$repoRoot\Directory.Build.targets" $temp + 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' + 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 + + + + $(MSBuildThisFileDirectory)packages\ + $(MSBuildThisFileDirectory)output\ + + + + + + +'@ + Set-Content "$temp\RuntimeDataOwner.proj" $project + Set-Content "$temp\MapWinGIS.ActiveX.proj" $project + $powerShell = (Get-Process -Id $PID).Path + & $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 + $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\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" } + } + + 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/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 e9891de..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 @@ -138,6 +139,19 @@ 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 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 `mwg::Viewport` (`mapwingis/map/Viewport.h`) is the view of map coordinates on @@ -387,7 +401,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..afd279a 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,27 @@ 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. +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 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 @@ -206,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 522a1ba..8ee628e 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -24,6 +24,19 @@ 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 `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 The native `MapWinGIS.ActiveX.Tests` project covers COM activation, registered diff --git a/docs/gdal-integration.md b/docs/gdal-integration.md index 043107a..7f2f19d 100644 --- a/docs/gdal-integration.md +++ b/docs/gdal-integration.md @@ -41,6 +41,18 @@ 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. 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. +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: @@ -68,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: @@ -89,6 +102,9 @@ 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()` accepts `-multi` and threaded warp +options such as `-wo NUM_THREADS=2`, with or without a progress callback. + ## ActiveX operations The ActiveX control exposes asynchronous generic GDAL execution through its @@ -213,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 ca278d8..73d148e 100644 --- a/docs/gdal-modern-capabilities.md +++ b/docs/gdal-modern-capabilities.md @@ -55,6 +55,15 @@ 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. +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. + `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. @@ -86,9 +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 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 c0f4ecc..a6a7458 100644 --- a/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp +++ b/src/MapWinGIS.ActiveX/GdiVectorLayerRenderer.cpp @@ -233,6 +233,23 @@ namespace mwg::activex return E_FAIL; } + 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)); if (brush.Get() == nullptr || pen.Get() == nullptr) 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 16dabcd..01d1531 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 @@ -66,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 @@ -78,8 +81,17 @@ 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(). + /// 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() + /// 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/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 d3a5f25..3137653 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. + /// 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 3ca027e..41ae5b4 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 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; [[nodiscard]] int FieldCount() const; @@ -95,7 +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. + /// 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); @@ -140,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 c8fd77f..faf016d 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,95 @@ namespace mwg }); } + GDALAlgorithm* GetActualAlgorithm(GDALAlgorithm* algorithm) + { + return algorithm != nullptr ? &algorithm->GetActualAlgorithm() : nullptr; + } + + struct PersistentOutput final + { + std::string path; + 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 = hasOutputName ? outputName : description; + if (dataset->GetAccess() == GA_Update) + result.openFlags |= GDAL_OF_UPDATE; + 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.path.empty() || output.path.starts_with("/vsistdout/")) + return {}; + + 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, + nullptr, + output.openOptions.empty() ? nullptr : openOptions.data(), + nullptr))); + } + void AppendErrors(std::vector& target, const GdalErrorScope& source) { const auto& errors = source.Errors(); @@ -100,6 +190,7 @@ namespace mwg { std::unique_ptr algorithm; GdalDatasetPtr outputDataset; + GdalDatasetPtr callerProvidedOutputDataset; std::vector errors; bool configurationValid = true; bool runAttempted = false; @@ -190,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) @@ -204,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) @@ -306,6 +399,12 @@ namespace mwg std::string(name) + "'."); _impl->configurationValid = false; } + else if (argument->GetType() == GAAT_DATASET && + argument->GetName() == "output") + { + _impl->callerProvidedOutputDataset.reset( + argument->Get().GetDatasetIncreaseRefCount()); + } return succeeded; } @@ -328,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(); @@ -399,19 +507,33 @@ namespace mwg // or threw after GDAL had already produced its output. _impl->executionSucceeded = succeeded; bool finalized = false; + std::optional persistentOutput; + GDALArgDatasetValue* outputValue = nullptr; + bool outputWasClosed = false; 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.get(); + if (!persistentOutput || callerProvidedOutput) + { + _impl->outputDataset.reset( + outputValue->GetDatasetIncreaseRefCount()); + } } 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(); @@ -426,6 +548,50 @@ namespace mwg "GDAL algorithm output could not be flushed."); } } + else if (finalized && !adapter.WasCancelled() && !HasFailure(_impl->errors) && + persistentOutput.has_value() && outputValue != nullptr) + { + 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) + { + AddFailureIfNoDiagnostic( + _impl->errors, + outputErrorCount, + "GDAL algorithm output could not be closed."); + } + else if (!HasFailure(_impl->errors)) + { + GdalErrorScope reopenScope; + _impl->outputDataset = ReopenPersistentOutput(*persistentOutput); + 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) + { + _impl->errors.push_back({ + GdalErrorClass::Warning, kRunnerErrorCode, + "GDAL algorithm output was written but no dataset could be reopened: " + + persistentOutput->path}); + } + } + } scope.RethrowCaptureException(); bool callbackFailed = false; @@ -462,11 +628,12 @@ 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) + if (output != nullptr && output->GetType() == GAAT_DATASET && !outputWasClosed) { outputClosed = output->Get().Close(); } 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 b8e31be..fb65490 100644 --- a/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp +++ b/src/MapWinGIS.Core/src/gdal/GdalUtils.cpp @@ -447,7 +447,6 @@ namespace mwg _impl->cancelled = false; if (HasNullByte(options)) return RecordFailure(_impl->errors, "GDAL utility options must not contain null bytes."); - 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 a18b200..e538a55 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 @@ -46,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"; @@ -141,8 +143,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 +191,66 @@ namespace mwg return GetFeature(index); } - void Save(OGRFeature* feature) const + void Save(OGRFeature* feature, const bool geometryChanged = false) const { - if (_layer->SetFeature(feature) != OGRERR_NONE) + GdalEnvironment::ClearLastError(); + const OGRErr result = _layer->SetFeature(feature); + if (geometryChanged) + { + _extent.reset(); + _extentNeedsExactRecalculation = true; + } + if (result != OGRERR_NONE) { - std::string message = "Cannot write feature " + std::to_string(feature->GetFID()); + const GIntBig featureId = feature->GetFID(); + _cachedFeature.reset(); + _cachedIndex = -1; + _featureIds.clear(); + _featureIdsValid = false; + _featureCount.reset(); + GdalEnvironment::RethrowLogException(); + std::string message = "Cannot write feature " + std::to_string(featureId); if (const std::string detail = GdalEnvironment::LastErrorMessage(); !detail.empty()) message += " (" + detail + ")"; throw CoreError(message); } - _extent.reset(); + if (geometryChanged) + _storedExtentDirty = true; + 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")) + { + 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) + { + if (result != nullptr) + _dataset.ReleaseResultSet(result); + }; + 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 @@ -228,14 +280,40 @@ 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; + 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 = GdalEnvironment::LastErrorMessage(); + const CPLErr errorType = CPLGetLastErrorType(); + if (errorType >= CE_Failure) + { + 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; @@ -255,7 +333,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(); @@ -285,30 +363,54 @@ namespace mwg _featureIdsValid = true; } - /// Tells the driver to skip attributes and geometry for the duration of - /// the id scan, and restores the layer afterwards. - class IgnoreAllButIds + [[nodiscard]] Extent ComputeExactExtent() const + { + OGRLayer& layer = *_layer; + const IgnoredFields ignoreScope(layer, _fields, false); + 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{}); + } + + /// 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; @@ -321,6 +423,8 @@ namespace mwg mutable std::int64_t _cachedIndex = -1; mutable std::optional _featureCount; mutable std::optional _extent; + mutable bool _extentNeedsExactRecalculation = false; + mutable bool _storedExtentDirty = false; }; std::int64_t detail::VectorLayerAccess::ForEachGeometry( @@ -545,7 +649,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 +666,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)))); } } @@ -615,12 +719,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 f9ceecc..bff67ae 100644 --- a/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp +++ b/tests/MapWinGIS.ActiveX.Tests/VectorLayerRendererTests.cpp @@ -553,6 +553,68 @@ 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, 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 87ef2ed..28b1b32 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,6 +240,16 @@ namespace CPLErr Close() override { + ++closeCount; + const CPLErr flushResult = FlushCache(false); + 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"); } @@ -254,6 +268,7 @@ namespace bool _failFlush; bool _failClose; + bool _reportCloseDiagnostic; }; class OutputFailureAlgorithm final : public GDALAlgorithm @@ -261,6 +276,10 @@ namespace public: static GDALDataset* outputDataset; static bool runSucceeded; + static bool rejectOpen; + static inline bool finalizeSucceeded = true; + static inline int openCount = 0; + static inline std::string lastOpenedPath; OutputFailureAlgorithm() : GDALAlgorithm( @@ -271,8 +290,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) { @@ -284,6 +313,8 @@ namespace _output.Set(output); outputDataset = nullptr; GDALReleaseDataset(output); + if (progress != nullptr) + (void)progress(1.0, "complete", userData); return runSucceeded; } @@ -291,8 +322,57 @@ 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: + 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 +394,10 @@ 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(); }); + registry.DeclareAlgorithm({"mwg-test", "worker-progress"}, + [] { return std::make_unique(); }); }); } @@ -608,6 +692,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(); @@ -654,8 +783,18 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS { return nullptr; } + ++OutputFailureAlgorithm::openCount; + OutputFailureAlgorithm::lastOpenedPath = path; + 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"); + const bool reportCloseDiagnostic = path.ends_with(":close-diagnostic"); + return new FailingOutputDataset(failFlush, failClose, reportCloseDiagnostic); }; GetGDALDriverManager()->RegisterDriver(driver.get()); @@ -664,25 +803,66 @@ TEST(GdalAlgorithmRunner, ReportsOutputFlushAndCloseFailuresAndAllowsSubsequentS const char* kind; const char* diagnostic; bool runSucceeded; + bool useGroupCommandLine; + bool rejectReopen; + bool finalizeSucceeded; + bool cancel; }; 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, 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{"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}}) { const std::string path = std::string("MWG_OUTPUT_FAILURE:output:") + failure.kind; 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; - GdalAlgorithmRunner failingRunner("mwg-test output-failure"); + 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"); + if (failure.useGroupCommandLine) + { + ASSERT_TRUE(failingRunner.ParseCommandLineArgs({ + "output-failure", "--output", path + })) << LastError(failingRunner); + } - EXPECT_FALSE(failingRunner.Run()); - EXPECT_NE( - failingRunner.Result().ErrorMsg().find(failure.diagnostic), - std::string::npos) - << failingRunner.Result().DetailedErrorMsg(); + 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; + 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; } @@ -703,6 +883,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(); @@ -740,6 +993,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 +1014,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..afa800b 100644 --- a/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp +++ b/tests/MapWinGIS.Core.Tests/GdalEnvironmentTests.cpp @@ -14,13 +14,21 @@ #include #include +#include #include "mapwingis/gdal/GdalEnvironment.h" +#include "TestData.h" 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; @@ -76,10 +84,31 @@ 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 +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 = ToUtf8(temp.Path() / "spatialite.sqlite"); + 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) { diff --git a/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp b/tests/MapWinGIS.Core.Tests/GdalUtilsTests.cpp index 793a8a0..b7dd16f 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,32 @@ TEST(GdalUtils, WarpsRasterUsingLegacyOptions) EXPECT_STREQ(dataset->GetSpatialRef()->GetAuthorityCode(nullptr), "3857"); } +TEST(GdalUtils, SupportsThreadedWarpOptionsWithProgress) +{ + GdalEnvironment::Initialize(); + GdalEnvironment::QuietScope quiet; + test::TempDirectory directory; + const std::filesystem::path input = test::MakeRaster(directory.Path()); + GdalUtils utils; + 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) { 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..0951a9c 100644 --- a/tests/MapWinGIS.Core.Tests/TestData.cpp +++ b/tests/MapWinGIS.Core.Tests/TestData.cpp @@ -42,10 +42,14 @@ 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); + 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); - if (!_state.ignoredFields && id != 1) + feature->SetField(0, _state.storedName.c_str()); + feature->SetField(1, _state.storedValue); + if (!_state.ignoredGeometry && id != 1) { OGRPolygon polygon; - if (id == 0) + if (id == 0 && !_state.onlyEmptyGeometries) { OGRLinearRing ring; ring.addPoint(0.0, 0.0); @@ -102,12 +108,18 @@ 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 { _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; } @@ -116,14 +128,45 @@ 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.warningExtentFailure) + { + CPLError(CE_Warning, CPLE_NotSupported, "Simulated unavailable extent"); + 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; @@ -140,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 @@ -154,7 +199,19 @@ namespace mwg::test return index == 0 ? &_layer : nullptr; } + 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; + if (_state.failRecomputeExtent) + CPLError(CE_Failure, CPLE_AppDefined, "Simulated extent recompute failure"); + return nullptr; + } + private: + VectorReadTestDriver& _state; VectorReadTestLayer _layer; }; } @@ -166,6 +223,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..c1c6eb2 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,26 @@ namespace mwg::test static constexpr const char* kPath = "MWG_VECTOR_READ_TEST:features"; 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 layerName = "features"; + std::string storedName = "Original"; + int storedValue = 0; int nextFeature = 0; int readCalls = 0; 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 afb4a62..3f32445 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) @@ -112,6 +114,110 @@ 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()); + const int readsAfterScan = driver.readCalls; + EXPECT_GT(readsAfterScan, 0); + EXPECT_TRUE(layer.GetExtent().IsEmpty()); + EXPECT_EQ(driver.readCalls, readsAfterScan); + 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(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(); @@ -247,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 @@ -408,6 +515,129 @@ 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(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); + 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); + 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); + 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); + EXPECT_EQ(driver.recomputeExtentStatement, "RECOMPUTE EXTENT ON road \"section\""); + 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, 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; + 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 = @@ -415,13 +645,39 @@ 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_P(VectorLayerEditTest, FlushPersistsShrinkingBoundsWithoutAnExtentQuery) +{ + { + 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 = Reopen(); + EXPECT_DOUBLE_EQ(reopened->Layer(0).GetExtent().xMax, 103000.0); +} + TEST_P(VectorLayerEditTest, KeepsGeneratedFixturesIndependent) { Edit([](VectorLayer& layer)