Cataclysm-DDA icon indicating copy to clipboard operation
Cataclysm-DDA copied to clipboard

Typified editmap

Open PatrikLundell opened this issue 1 year ago • 6 comments

Summary

None

Purpose of change

Changed untyped (tri)points to typed ones. This time focusing on editmap.h/cpp.

Describe the solution

Go through the code and figure out what types the untyped stuff is. Change it and update used operations to work.

Describe alternatives you've considered

Testing

  • Loaded a save and walked away a bit to see some local wildlife.
  • Walked back and up to the roof.
  • Used the editmap debug functionality to create field, terrain, and furniture, as well as an overmap and an overmap special (the latter with the overmap editing), since editmap was the code modified.

Additional context

It got messy with line, as introducing typed operations resulted in circular header copying. Addressed that by introducing a new header file containing the typed operation while the actual implementation remains in line.cpp.

The implementation is ugly, because the bresenham stuff generates a vector, and so cannot easily be coaxed into generating a vector of a typed type, nor is it an easy conversion to deal with the resulting vector. I suspect it can be dealt with via modern macros ("lambda"?), but I can't do it.

It can also be noted that there are other cases where side header files may be needed to avoid circular dependencies, and this may be needed for future conversions (earlier conversion has worked around these issues by converting the result from the untyped implementation. I have a vague feeling that bresenham has caused this kind of trouble earlier).

PatrikLundell avatar May 12 '24 14:05 PatrikLundell

The code won't link, but I can't figure out why. It linked fine as long as I forgot to include the new header file in line.cpp (with a complaint that the new function should be static if it didn't have a header). However, when I included the missing file there's a whole range of errors from xmemory claiming tripoint_bub_ms doesn't match any instantiation of the tripoint template. The compiler is of no help, as it finds the the thing's specification just fine. It's presumably caused by some esoteric aspect of C's header copying antics, but I have no idea how to resolve it.

PatrikLundell avatar May 12 '24 21:05 PatrikLundell

Is there some syntax to tell the code checker that yes, there is an unused parameter here, but it has to be there?

Edit: Testing a syntax that shouldn't be legal based on how dummy pointers are used in override operations. We'll see if that appeases the style demons.

PatrikLundell avatar May 13 '24 08:05 PatrikLundell

The problem here is that the 2D version has a single optional parameter while the 3D version has 2 optional parameters, but the macro still think it can call the 2D one with two additional parameters (which probably is incorrect if there are any 2D callers).

I think the correct solution would be providing different specializations when the Point template argument is point or tripoint.

I think it's a bad idea to have this stuff in coordinates.h

I guess it might have been done that way to ensure line_to's dependencies in coordinate.h are always declared before it.

Qrox avatar May 13 '24 10:05 Qrox

I tried to define two templates with different profiles, but that's thwarted by the default parameters that result in both of them matching all the usage, and thus making it ambiguous. I don't understand C templates, but it seems you toss out some text and the compiler tries to match whatever to it, so there wouldn't be any way to specify you only want point derivatives, or only tripoint derivatives. I can be wrong, of course, but my attempt failed.

I don't see why placing code in coordinates.h would provide anything that a coordinates_operations.h including both line.h and coordinates.h wouldn't. Headers have no use for the line operations, and so would benefit from being able to include just the definitions of the specific types, without the code baggage that also drags additional header file baggage with it.

PatrikLundell avatar May 13 '24 13:05 PatrikLundell

I think you can use std::enable_if to ensure each template only matches point or tripoint.

coordinate.h includes lines.h so when the former is included before the latter in a cpp file, lines.h comes first in the translation unit, so if the template function line_to is defined in lines.h, it comes before the declaration of its dependencies in coordinate.h, which may result in errors. I do remember in some cases it is fine for dependencies to come after a declaration, so maybe it's fine to move line_to to lines.h, but I haven't tested.

Qrox avatar May 14 '24 02:05 Qrox

I've tried to figure out how to use enable_if, but to no avail (I've tried a lot of different permutations, and none seem to work). I don't know what to place in the test, the format of the test, or even where to place the test. As far as I can tell the best I've achieved is to disable matching (where it should have matched), resulting in complaints that a typed tripoint can't be converted to tripoint (which I think is caused by skipping the template and trying to match the operation in line.h).

PatrikLundell avatar May 14 '24 12:05 PatrikLundell

Thanks! I got somewhat close, but not quite there... There's one more thing I'd like to do. It's cosmetic, but I want to change Point to Tripoint in the tripoint specific template for clarity.

I'm in the middle of testing another PR, but expect to get there during the day.

PatrikLundell avatar May 14 '24 15:05 PatrikLundell