gdal_trace_outline ignores alpha layer

[BrannonKing]

I branched the code and modified ndv.cc to handle alpha layers. Mind taking a look? I'm not sure my approach was right, though it does work for me in my situation.

Also in my branch: some changes to use C++11 to avoid Boost references, changes to make the major ring and holes parameters work simultaneously, and an attempt to make erosion parameterizable.

Link: https://github.com/asirobots/dans-gdal-scripts

[dstahlke]

Brannon, Thanks for the patch. I haven't touched this project in years, so it is nice to at the very least have some of it updated to C++11. I think the patches need some work though before being merged. Do the added VC files serve some purpose that will be of use to others? If so, are you willing to be a contact for people who are trying to build on Windows? I know nothing about Windows development, and so can't support this myself and can't judge whether these files really should be included. In common.h, the #include "config.h" has to be up top. In particular, it can't be inside of " #if HAVE_INTTYPES_H" since that symbol comes from config.h. Bring back boost::lexical_cast. It has better error handling than stoi and its ilk in cases of improperly formatted strings. Outputting donuts by default when -major-ring is specified is definitely a change in behavior. Probably it's the right thing to do, because the user can also specify the -no-donuts option if they want. But you need to remove this check: if(major_ring_only && output_no_donuts) fatal_error( "-major-ring and -no-donuts options cannot both be used at the same time"); Please also test that it does the right thing with and without -no-donuts in the case where -major-ring is specified. You can use the images under src/tests to test this. In take_largest_ring, it needs to be " if(mp_in.rings[i].is_hole && mp_in.rings[i].parent_id == best_idx)". And the "if(i==best_idx) continue" is not needed. For consistency, remove spaces before parentheses in "if (" and "for (". Bring back "include <string>" in ndv.h and polygon-rasterizer.cc. These use std::string. Bring back boost::format in raster_features.cc. I think it's clearer than the hexmap[] stuff. The ones that have at most one std::to_string per line are probably more clear with std::to_string though. I don't understand the alpha band logic. Why is it multiplied by 0.25? Shouldn't only full transparency be considered non-data? If the user wants a more fuzzy match, they can specify this with the -ndv option. Is it good to add an NdvSlab when GDALGetRasterNoDataValue fails for some of the bands? I don't have much opinion either way on this, but it should be consistent. As it stands with your current patch, if this call fails for one band then there will be an NdvSlab added only if one of the other channels is alpha. Either it should always add NdvSlab if at least one band has GDALGetRasterNoDataValue, or it should not (regardless of whether another band is alpha). Also, I'm not so sure the logic is sound here. If the file lists [-1,-1,-1] as being NDV (for example), wouldn't you want two NdvSlabs? One for the listed NDV value and another for transparent pixels. As it is now, it'd only be NDV if the pixels were equal to [-1,-1,-1] and also transparent. Instead of the modified line " if(!got_error || alpha_idx >= 0)", it would be better to not set got_error in cases where the slab should be added. Thanks, Dan

…

On 01/23/2017 04:11 PM, Brannon King wrote: I branched the code and modified ndv.cc to handle alpha layers. Mind taking a look? I'm not sure my approach was right, though it does work for me in my situation. Also in my branch: some changes to use C++11 to avoid Boost references, changes to make the major ring and holes parameters work simultaneously, and an attempt to make erosion parameterizable. Link: https://github.com/BrannonKing/dans-gdal-scripts/commits/master — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#11>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATHcOs2-Lw43WrYQxayTTDMF-KWSwskks5rVUGWgaJpZM4LrtXH>.

[BrannonKing]

Thanks for the thorough review. I'm aware more work would need to be done for a full merge.

The Visual Studio files are an okay example of how to do it. I was using those in conjunction with prebuilt binaries from http://www.gisinternals.com/ . A lot of modern projects have switched from autoconf to CMake because CMake will generate Visual Studio build files on demand. Using CMake would allow us to leave the Visual Studio project files out of the repository.

I wasn't sure what to do on the config.h reference. I suppose I could have done something like wrap it in #ifndef _WINDOWS...

I had intended to use std::stoi, not the old stdlib version. The newer conversion methods do throw errors on malformatted input. I was concerned about getting the right version of Boost to match whatever GISinternals used on their build, which wasn't obvious. It seemed easier to just ditch it. It might be nice, though, to utilize newer build chain features for referencing individual Boost features straight from Github -- some kind of auto external dependency.

On the alpha, I did wonder if any alpha meant "keep it". I also wondered if 50% was a good cutoff. A zero alpha threshold would probably work fine in all me test data.

Concerning simultaneous NDV entries, I had expected that your code would OR them. You're saying it's an AND, so my logic there is definitely not quite right.

[dstahlke]

On 01/24/2017 06:48 AM, Brannon King wrote: Thanks for the thorough review. I'm aware more work would need to be done for a full merge. The Visual Studio files are an okay example of how to do it. I was using those in conjunction with prebuilt binaries from http://www.gisinternals.com/ . A lot of modern projects have switched from autoconf to CMake because CMake will generate Visual Studio build files on demand. Using CMake would allow us to leave the Visual Studio project files out of the repository.

Feel free to write up some CMake scripts if you'd like. I'm not particularly tied to autoconf. As long as it still compiles on Linux, it's fine.

I wasn't sure what to do on the config.h reference. I suppose I could have done something like wrap it in |#ifndef _WINDOWS...|

config.h comes from autoconf. You can look at config.h.in to see the sorts of things it defines. If all references to these are removed from the code, then config.h will not be needed. A lot of it just detects the existence of headers which pretty much exist on all systems nowadays.

I had intended to use |std::stoi|, not the old stdlib version. The newer conversion methods do throw errors on malformatted input. I was concerned about getting the right version of Boost to match whatever GISinternals used on their build, which wasn't obvious. It seemed easier to just ditch it. It might be nice, though, to utilize newer build chain features for referencing individual Boost features straight from Github -- some kind of auto external dependency.

std::stoi also has pretty crummy error detection, unless you pass the pos parameter, and manually check its result. For instance std::stoi("1O1") will return 1 rather than throwing an error. Since we're only using boost headers, and not the compiled libraries, it is probably okay to mix versions.

On the alpha, I did wonder if any alpha meant "keep it". I also wondered if 50% was a good cutoff. A zero alpha threshold would probably work fine in all me test data. Concerning simultaneous NDV entries, I had expected that your code would OR them. You're saying it's an AND, so my logic there is definitely not quite right.

Yes, if my memory serves, it operates like an AND between the bands. Imagine it as a box in color space. You can review the code to be sure though. It's been several years since I've looked at it. And I'd say by default semi-transparent pixels should be considered valid data. It seems that would be in line with the principle of least surprise. This can always be configured on the command line with the -ndv option.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub <#11 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATHcHyYeG59H3qyFFIqT1Z9OE83t5rYks5rVg9YgaJpZM4LrtXH>.

[BrannonKing]

Adding an update that I no longer use the MS Build files in my branch. For Windows, I compile things directly with a call like this one:

cl.exe /MD /O2 /EHsc /Fe:gdal_trace_outline /DNDEBUG /D_CONSOLE ^
  src\beveler.cc src\common.cc src\datatype_conversion.cc src\debugplot.cc src\dp.cc ^
  src\excursion_pincher2.cc src\gdal_trace_outline.cc src\georef.cc src\mask-tracer.cc ^
  src\mask.cc src\ndv.cc src\palette.cc src\polygon-rasterizer.cc src\polygon.cc ^
  src\raster_features.cc src\rectangle_finder.cc ^
  /I..\gdal\gdal\ogr /I..\gdal\gdal\port /I..\gdal\gdal\gcore /I..\gdal\gdal\ogr\ogrsf_frmts ^
  /link ..\gdal\gdal\gdal_i.lib