Clean up WriteImage functions

[der-stefan]

Currently, splat.cpp contains four functions for writing raster map outputs:

• WriteImage
• WriteImageDBM
• WriteImageLR
• WriteImageSS

Most of their code is identical, just the details differ. It would make sense to combine those in one function. That would make it easier to include new image formats (like GeoTiff or World Files, see #8 ). edit: was no problem.

[hoche]

I thought I had a note somewhere to address this, but I can't seem to find it. Yes, it's silly to have this much code replication and this should be refactored. Right now the main differences in the four functions are a) where they get their data, and b) color maps used for that data. The first can probably be solved with some set of callback functions, and the second can be a set of lookup tables.

There're other ways to do it, but that's what I came up with off the top of my head.

[watkipet]

I'd like to try tackling this unless someone else wants to. I like the idea of lookup tables. Instead of callback functions, I think I'd turn the existing Image class into an abstract base class with a Draw function. Then I'd make ImageLOS, ImageDBM, ImageLR, and ImageSS concrete subclasses. Hopefully Draw can call some pure virtual functions implemented by the concrete classes which will provide the data. So yeah, I guess it's function pointers with a lot of C++ syntax. :)

There's an extra type of output that can go along with the images, though: KML, KMZ, PGW ( #8 ) and possibly an interactive map ( #5 ). All of these depend on the filename of the image and the image dimensions. I don't think the generation of these metadata files belongs in the Image class. However, whatever generates them should be able to consume any concrete Image object and gain enough data to generate the appropriate metadata file.

Thoughts?

[hoche]

I was sort of leaning towards extending the Report class to drive this at a high level, where a report is a collection of one or more output files. I was trying to figure out a way to separate the number-crunching from the generated output though; right now sometimes we do the area calculations and sometimes we do N linear ones, and it's the kind of output that's selected that drives that.

[watkipet]

I think I might see what you're proposing, but I'm not positive. Would the proposed Report class also contain the image output files? Would it have flags indicating all the reports that should be generated or would that remain in the SplatRun class?

Right now Report just generates different types of text output, and as you mentioned, does some calculations as well. Most of the other calculation is in the ElevationMap class--it's kind of the calculation canvas / working space.

Is the first step in all this pulling the calculations out of the Report class?

[der-stefan]

The clean-up is mostly done. However, I did not include the LoS yet, as it currently outputs just a black image. So the existing function "Image::WriteImage()" is non-functional. Could somebody give me a hint what's going wrong there?

[watkipet]

@der-stefan, one thing that's going wrong with Image::WriteImage() are these lines:

for (int y = 0, lat = north; y < (int)height; y++, lat = north - (sr.dpp * (double)y)) { for (int x = 0, lon = em.max_west; x < (int)width; x++, lon = (double)em.max_west - (sr.dpp * (double)x)) {

The compiler is treating both y and lat as type int (same with x and lon). To fix it, declare these above the loop (as they aren in the other WriteImage* methods). Not sure if that's the black image problem--but it is a problem. It's making it so the you just get 4 or so very large squares of a single color with PPM output. Fix the above and the PPM output should be fixed.

[watkipet]

I finished fixing the LOS image writing in the json branch. I know this doesn't address having a general "Report" class that handles images too--but perhaps we can open a separate issue for that and close this one? Everyone OK if I close this as fixed?

[der-stefan]

Great that you found the bug! I am not aware of any guilt, this bug must have been introduced earlier...
The issue is mainly solved. However, I am not completely satisfied with the solution that the "dem" pixels (object should be renamed to "result" or similar...) hold different values for loss, dBm or dBµV/m outputs. A comment in the function PlotLRPath() gives the hint that the author wants to scale the result roughly between 0 and 255. As Splat is not meant to run on a microcontroller, it is alright to spend 32 bit each. So if the output format is just considered in the WriteImage function, it would be a much cleaner approach and ease outputting all formats at a time. The result array should hold loss values in general. With this you can even sweep the TX power within fractions of a second e.g. to visualize the range, which is defined by a receiver sensitivity.

To sum things up: I would like to clean up WriteImage() and the functions in "elevation_map.cpp" further and pull the logic outwards.

[watkipet]

@der-stefan - I would like to clean up and separate out concerns for WriteImage and ElevationMap as well. However, I think that should be a separate issue. It needs some discussion first. Mind if I close this one and make a new one where we can discuss how ElevationMap, Dem, and Image should be modified? For one thing, ElevationMap isn't really just an elevation map right now--it's the working canvas (mostly) where (most of) the calculations take place. I have lots of ideas and thoughts I'd like to discuss, but shall we do it in a new issue?

[der-stefan]

Exactly, I am always confused where to find which functionality...
Yeah, go ahead!