GDAL DEM Processing Routines

[metasim]

• I agree to follow the project's code of conduct.
• I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

---

• Add hillshade
• Add color-relief
• Wait for and merge Added Extend and various FromIterator implementations to CslStringList. #457
• Remove DemCommonOptionsOwner and inline the fields into the various structs.
• Remove DemCommonOptions
• Remove Option from around DemCommonOptions::compute_edges and additional_options.
• Convert DemProcessing methods to inherent methods on Dataset.
• Convert DemProcessing methods to free functions.

[metasim]

@lnicola Thank you for the early feedback. Course corrections earlier rather than laters are definitely helpful.

[lnicola]

By the way, if it's in a trait you also get completions for it. Not that I'm proposing this, just wanted to point it out.

[lnicola]

I'm inclined to say we should avoid Option for most, if not all of these. I understand that the default might change, but if -zero_for_flat ever becomes the default, there will be a new -non_zero_for_flat and we'll have to update the code anyway. That is,

        if self.zero_for_flat == Some(true) {
            opts.add_string("-zero_for_flat").unwrap();
        }

already assumes that the default is false. algorithm is probably an exception here, since the default is not documented.

But we can probably change them later, you must be sick of this PR by now.

[lnicola]

We don't have getters for these, but the common options have them.

[metasim]

It was oversight, but we're removing getters, right?

[lnicola]

Same as for store_common_options_to, but more user-facing we could return Result<CslStringList> here and propagate them nicely out of dem_eval. These are unlikely to fail but might, for example on OOM.

Slight binary size win and fewer reasons for people to yell that we're bad for doing unwrap (it happens more often than one might think).

[lnicola]

metasim@39ed430

[lnicola]

And one final question is where to put these. They're currently under raster::processing::dem, but we also have programs::raster. I think I like programs more, since it's a little weird to have raster::processing::translate and vector::processing::vector_translate.

Another aspect is that I think the Rust guidelines encourage tall/deep module hierarchies, but with re-exports so that the user only sees shallow ones.

[metasim]

And one final question is where to put these. They're currently under raster::processing::dem, but we also have programs::raster. I think I like programs more, since it's a little weird to have raster::processing::translate and vector::processing::vector_translate.

The rationale for this is two fold:

1. I plan to work on the Warp subsystem, and slotted it to live in raster::processing:warp. But that's separate from the export structure. See below.
2. I don't think programs makes any sense in a library. If I'm looking at a crate structure, I'd expect programs be the same as bins: a place to put source for executable targets. From the outside I wouldn't know how to semantically interpret programs. Heck, even from the inside I don't know how to interpret programs. Long term, I think the contents of the programs module should be migrated to something more logical.

On your point about raster::processing vs programs::raster, I'd argue the latter breaks the convention in the main library by reversing scope. We train users to think "domain first, capability second", but with programs::raster it's "if I don't find it in raster::, I should also look in programs::raster. But because I find programs confusing, perhaps I'm missing a larger point.

I can see arguments against raster::processing, but to fix its weaknesses I think we need to think more holistically about module structure and what heuristics we use to determine the proper destination for a capability. If the answer is "do exactly what is done in C++" (which would be valid) then we have even more of a gap to address. But I think we can do better than that. To me, raster::processing is a place for advanced capabilities beyond opening and querying Datasets.

Another aspect is that I think the Rust guidelines encourage tall/deep module hierarchies, but with re-exports so that the user only sees shallow ones.

My intent was the use would use gdal::raster::processing::dem::*, since DEM stuff is kinda niche. But I see an argument that this is too deep. I don't really think it should be exported at the gdal::raster::* level. Again, programs::raster doesn't makes sense to me as "what's a 'program' here?".

How about this: we close the PR as it is (or tweak the pub use structure), and open a new ticket to discuss a design around the organization of all constituent parts?

[lnicola]

I don't think programs makes any sense in a library. If I'm looking at a crate structure, I'd expect programs be the same as bins: a place to put source for executable targets. From the outside I wouldn't know how to semantically interpret programs. Heck, even from the inside I don't know how to interpret programs. Long term, I think the contents of the programs module should be migrated to something more logical.

I agree it's unusual, but many of the GDAL "apps" are already exposed as a library (everything that ends up taking an argv as an input, including GDALDEMProcessing. I'm not that familiar with them, but I think they're split between gdal_alg.h and gdal_utils.h in the GDAL headers.

But because I find programs confusing, perhaps I'm missing a larger point.

My thinking is "I want gdalwarp, that's a "program". I want (to give a random example) ExecuteSQL, that's not a program". Even today, raster is a bit of a kitchen sink: https://docs.rs/gdal/latest/gdal/raster/index.html.

How about this: we close the PR as it is (or tweak the pub use structure), and open a new ticket to discuss a design around the organization of all constituent parts?

That will never get anywhere, so please no 😛.

[metasim]

"I want gdalwarp, that's a "program".

gdalwarp barely exposes the potential of the Warp subsystem. After reading through a lot of the GDAL Warp code over the last year, I think it's potential is barely tapped by the gdalwarp executable, and limited by the lack of examples and complex options model. It may be wishful/idealistic thinking on my part, but with Rust, we could expose a lot of the processing potential in a much less intimidating way than it it is in C++. Rust enabled pixel functions alone could be very powerful. That's my dream anyway.

[metasim]

👍

[metasim]

bors r=lnicola