Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typified editmap #73713

Merged
merged 12 commits into from
May 14, 2024
Merged

Typified editmap #73713

merged 12 commits into from
May 14, 2024

Conversation

PatrikLundell
Copy link
Contributor

@PatrikLundell PatrikLundell commented May 12, 2024

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).

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions labels May 12, 2024
@PatrikLundell
Copy link
Contributor Author

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.

@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label May 12, 2024
src/line.cpp Outdated Show resolved Hide resolved
src/line_coordinates.h Outdated Show resolved Hide resolved
PatrikLundell and others added 2 commits May 13, 2024 07:05
Co-authored-by: Jianxiang Wang (王健翔) <qrox@sina.com>
@PatrikLundell
Copy link
Contributor Author

PatrikLundell commented May 13, 2024

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.

@Qrox
Copy link
Contributor

Qrox commented May 13, 2024

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.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label May 13, 2024
@PatrikLundell
Copy link
Contributor Author

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.

@Qrox
Copy link
Contributor

Qrox commented May 14, 2024

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.

@PatrikLundell
Copy link
Contributor Author

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).

Copy link
Contributor

@Qrox Qrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work. The std::enable_if ensures that only one template declaration is valid for point and tripoint.

src/coordinates.h Outdated Show resolved Hide resolved
src/coordinates.h Outdated Show resolved Hide resolved
src/line.cpp Outdated Show resolved Hide resolved
src/line.h Outdated Show resolved Hide resolved
PatrikLundell and others added 4 commits May 14, 2024 17:56
Co-authored-by: Jianxiang Wang (王健翔) <qrox@sina.com>
Co-authored-by: Jianxiang Wang (王健翔) <qrox@sina.com>
Co-authored-by: Jianxiang Wang (王健翔) <qrox@sina.com>
Co-authored-by: Jianxiang Wang (王健翔) <qrox@sina.com>
src/coordinates.h Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the astyled astyled PR, label is assigned by github actions label May 14, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@PatrikLundell
Copy link
Contributor Author

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.

@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label May 14, 2024
@dseguin dseguin merged commit 7467c42 into CleverRaven:master May 14, 2024
21 of 26 checks passed
@PatrikLundell PatrikLundell deleted the typify branch May 15, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants