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

Refactor out Crd3D and Crd2D to use df::coord and df::coord2d #207

Closed

Conversation

realSquidCoder
Copy link
Collaborator

Fixes #205

}

Crd2D LocalTileToScreen(int32_t x, int32_t y, int32_t z)
df::coord2d LocalTileToScreen(int16_t x, int16_t y, int16_t z)
{
pointToScreen((int*)&x, (int*)&y, z);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is casting a int16_t* to int*.

@ab9rf
Copy link
Member

ab9rf commented Feb 11, 2025

I'm going to stand here in opposition to the entire idea behind this PR. df::coord and df::coord3d are garbage types and we should use them only to the extent possible required to remain compatible with DF. In code that is doing its own manipulation of 2-tuples and 3-tuples of ints representing coordinate vectors, we should absolutely not use df::coord because this could result in slower code. The compiler is going to use 32-bit math under the hood anyway, and constantly forcing narrowing and widening conversions is of no benefit. The only time you should be using int16_t is when memory bandwidth is an issue, and that's not the case for singleton or small lists of 3-tuples which is the overwhelming use case here.

I would instead recommend keeping these types mostly as is, and add the required constructors and cast operators to the Crd2D and Crd3D types so that they are implicitly interconvertible with df::coord objects, to the extent this is useful. If it is needed to represent "invalid" values, add an isValid boolean or wrap in std::optional (which does the same thing and adds a convenient API)

@realSquidCoder
Copy link
Collaborator Author

im completely in agreement with this. this PR broke more than it fixed.

@realSquidCoder
Copy link
Collaborator Author

This is not gonna happen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Developer Request: Use df::coord and df::coord2d instead of Crd3D and Crd2D
3 participants