Add GDALDemProcessing, other cleanup #54
Conversation
Lazy evaluation has made it more difficult to track down the source of errors because all operations on a dataset are chained together and executed at once. This adds a prefix to any errors emitted by GDAL that explains where the error occurred.
There was a problem hiding this comment.
Really nice effort here 🎉. Code reads nicely, comments are descriptive about what and why things are happening without being duplicative, and function input limitations described by the README docs are checked in tests and code. Really appreciate splitting commits into logical chunks, made this PR easy to follow.
src/wrappers/gdalDemProcessing.js
Outdated
| let filePath = directory + '/' + filename; | ||
|
|
||
| // And then we can kick off the actual processing. | ||
| // TODO: The last parameter is an int* that can be used to detect certain kinds of errors, |
There was a problem hiding this comment.
Should this TODO be an issue to investigate?
There was a problem hiding this comment.
Good call, I think this should more just not be a TODO at all -- I've been able to get pretty good error messages out of the CPLGetLastError* methods, so it hasn't felt necessary to dig into what this does so far.
src/wrappers/gdalDemProcessing.js
Outdated
| import guessFileExtension from '../guessFileExtension.js'; | ||
| import ParamParser from '../stringParamAllocator.js'; | ||
|
|
||
| // TODO: This is a good reason to switch to Typescript |
There was a problem hiding this comment.
Could also make "Switch to Typescript" an issue or epic, remove this comment, and put it in the issue as an example of a "pro".
There was a problem hiding this comment.
Good call, absolutely; I've had Typescript in my head for a while but it has never made it into a written issue. Just added #55 and will update this.
| **Note**: This returns a new `GDALDataset` object but does not perform any immediate calculation. Instead, calls to `.render()` are evaluated lazily (as with `convert()` and `warp()`, above). The render operation is only evaluated when necessary in order to access some property of the dataset, such as its size, bytes, or band count. Successive calls to `.warp()` and `.convert()` can be lazily chained onto datasets produced by `.render()`, and vice-versa. | ||
| #### Parameters | ||
| - `mode`: One of ['hillshade', 'slope','aspect', 'color-relief', 'TRI', 'TPI', 'roughness']. See the [`gdaldem documentation`](https://gdal.org/programs/gdaldem.html#cmdoption-arg-mode) for an explanation of the function of each mode. | ||
| - `args`: An array of strings, each representing a single [command-line argument](https://gdal.org/programs/gdaldem.html#synopsis) accepted by the `gdaldem` command. The `inputdem` and `output_xxx_map` parameters should be omitted; these are handled by `GDALDataset`. Example: `ds.render('hillshade', ['-of', 'PNG'])` |
There was a problem hiding this comment.
Ensuring that inputdem and output aren't in this array when a user calls the function doesn't seem to really be a thing that could be validated in code, right? You'd just be looking for a string filename and presumably if you passed them in args the existing error handling would throw with a somewhat helpful error?
There was a problem hiding this comment.
This is a good question -- I don't know what will happen if the user erroneously puts in an input or output filename. I'll write a test for it and see what happens. But yeah, my hope is that the GDAL argument parsing will be able to provide helpful feedback in this case, because it would basically end up with two of them.
There was a problem hiding this comment.
Looks like it says something along the lines of Too many command options 'input.tif', which is not totally clear about what's going on but at least identifies the problematic argument correctly. I think seeing that would drive most users to try removing the input.tif option as a first step, and that would be the right thing to do even though the explanation wouldn't have reinforced that the filenames are handled internally.
There was a problem hiding this comment.
As a note, I'm not including the test I wrote because all the relevant functionality is on the GDAL side so the test wouldn't exercise any Loam code that doesn't already have a test.
There was a problem hiding this comment.
That error seems to be good enough for now. If this trips people up in the future, we could look for the "Too many command options" part of the error message and append something like "Be sure you're not including your input and output tif paths to args." to it. Otherwise, like you said, doesn't seem to be much can be done.
ddohler
force-pushed
the
feature/dpd/gdal-dem-processing
branch
from
b54bd55
to
e92502f
Compare
|
Thanks for the review! I've updated based on the comments and I'm going to go ahead and merge. |
Overview
Primarily adds a wrapper for GDALDemProcessing, but also includes a fair bit of code cleanup; I've tried to break logically separate changes into separate commits as much as possible.
Demo
Here's a demo of the new function in action. The backing data is from the Wildfires application, which requires dynamic color relief shading of a raster representing modeled wildfire risk. I've converted the single GeoTIFF into a tile pyramid, but with each tile delivered in GeoTIFF format and then dynamically rendered on the frontend. Let me know if you want to play with a live link.
Notes
I'm going to release Loam 1.0.0 after this is merged. I'm close to having a blog post and demo written up, and I'd like to move on to other things for the time being (although I have plenty more ideas for Loam!).
Testing Instructions
gdaldemon the command-line with the test input file to confirm the byte counts are accurateChecklist