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

2D Coordinate Type #103

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

2D Coordinate Type #103

wants to merge 4 commits into from

Conversation

dxrcy
Copy link
Contributor

@dxrcy dxrcy commented Nov 2, 2024

Introduces a new '2D coordinate' struct which mirrors the existing Coordinate struct, but does not contain a y value.
Adds constructors to (often implicitly) convert between Coordinate and Coordinate2D.

Replaces some parameters and return values with Coordinate2D in relevant functions (getHeight and getHeights in MinecraftConnection, get_worldspace and base_pt in `HeightMap).

Possible changes:

  • Rename to Coordinate2 (mirrors naming style for other libraries, eg. Vec2, float3). Likely not ideal since Coordinate keeps the same name regardless.
  • Update documentation for description of Coordinate2.

Note that overloading function parameters such as MinecraftConnection::getHeight to accept a Coordinate is unnecessary, since a Coordinate will be implicitly cast to a Coordinate2 in such a context.

@rozukke rozukke added documentation Improvements or additions to documentation enhancement New feature or request next-release To be released next time breaking changes are allowed labels Nov 3, 2024
@dxrcy dxrcy marked this pull request as ready for review December 3, 2024 08:08
@@ -34,8 +34,7 @@ int main() {
int min_height = *std::min_element(heights.begin(), heights.end());

// Build rings, diminishing up to pyramid height
mcpp::Coordinate base_pt = heights.base_pt();
base_pt.y = min_height;
mcpp::Coordinate base_pt = {heights.base_pt(), min_height};
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure how I feel about this constructor... ordering seems important for an ordered x,y,z coordinate

@rozukke
Copy link
Owner

rozukke commented Dec 5, 2024

Also, something like a filled() or with_height() method would be nice for Coordinate2D as a quick shorthand to get areas of non air blocks - for example:

auto c1 = Coordinate2D(0, 0);
auto c2 = Coordinate2D(100, 100);
auto chunk = mc.getBlocks(c1.filled(), c2.filled());

where each filled() call does a getHeight() under the hood.

@dxrcy
Copy link
Contributor Author

dxrcy commented Dec 6, 2024

where each filled() call does a getHeight() under the hood.

How could it do this without accessing the MinecraftConnection?

I suppose Coordinate2D::filled could return a new type which getBlocks has an overload for, but this seems unintuative.

Perhaps withHeight could be a method of MinecraftConnection, to convert a Coordinate2D to a Coordinate:

Coordinate2D c1{0, 0};
Coordinate2D c2{100, 100};
Chunk chunk = mc.getBlocks(
    mc.withHeight(c1),
    mc.withHeight(c2)
);

Although this doesn't read as nice (grammatically), so withHeight could have a different name.

Note that getHeight and withHeight would be essentially the same, but withHeight would return the entire coordinate. This would not be possible as an overload of getHeight, unless some flag parameter is passed.

Alternatively, getBlocks could accept Coordinate2D arguments, but I think this would lead to more confusion.

@dxrcy dxrcy changed the title 2D Coordinate Type (Draft) 2D Coordinate Type Dec 6, 2024
@dxrcy
Copy link
Contributor Author

dxrcy commented Dec 6, 2024

I think mc.fillHeight would be a good name instead.

@rozukke
Copy link
Owner

rozukke commented Dec 10, 2024

Ah, my bad. Didn't think through that dependency properly. Your suggestion sounds good (with snake case though).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request next-release To be released next time breaking changes are allowed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants