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

cmap #472

Open
wants to merge 12 commits into
base: fabric
Choose a base branch
from
Open

cmap #472

wants to merge 12 commits into from

Conversation

wagyourtail
Copy link
Contributor

@wagyourtail wagyourtail commented Dec 10, 2022

save held map to file

Copy link
Collaborator

@xpple xpple left a comment

Choose a reason for hiding this comment

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

Cool command, just a few code style changes mostly.

@xpple
Copy link
Collaborator

xpple commented Jan 8, 2023

I still like @haykam821's suggestion of extending the functionality of this command to maps you are looking at. Should I open a separate issue for that?

@wagyourtail
Copy link
Contributor Author

he can pr that xd

@wagyourtail
Copy link
Contributor Author

that was much harder than expected but it works

@wagyourtail wagyourtail requested review from xpple, Earthcomputer and haykam821 and removed request for Earthcomputer and xpple January 9, 2023 03:52
@wagyourtail
Copy link
Contributor Author

wait, since when can I not request multiple reviews

Comment on lines 67 to 68
int width = mapInfo[0].length * 128 * upscale;
int height = mapInfo.length * 128 * upscale;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could 128 be replaced with size constants? These constants could be initialized to FilledMapItem.field_30907 and FilledMapItem.field_30908.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which one is which

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd assume FilledMapItem.field_30907 is width and FilledMapItem.field_30908 is height.

Copy link
Collaborator

@xpple xpple left a comment

Choose a reason for hiding this comment

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

Some style changes.

Comment on lines +32 to +35
import static com.mojang.brigadier.arguments.IntegerArgumentType.getInteger;
import static com.mojang.brigadier.arguments.IntegerArgumentType.integer;
import static net.fabricmc.fabric.api.client.command.v2.ClientCommandManager.argument;
import static net.fabricmc.fabric.api.client.command.v2.ClientCommandManager.literal;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe your latest commit may have turned these back into non-wildcard imports again.

Comment on lines +42 to +49
dispatcher.register(literal("cmap")
.then(literal("export")
.executes(ctx -> exportMap(ctx.getSource(), 1))
.then(argument("upscale", integer(1, 16))
.executes(ctx -> exportMap(ctx.getSource(), getInteger(ctx, "upscale")))
)
)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It has become convention to not put the closing parentheses on a newline.

int y = vec.getComponentAlongAxis(yAxis.getAxis()) * yAxis.getDirection().offset();
int x = vec.getComponentAlongAxis(xAxis.getAxis()) * xAxis.getDirection().offset();
// this should never be possible
if (flattened.put(new Vec2i(x, y), next) != null) throw new IllegalStateException("Duplicate item frame at " + nextPos);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add curly brackets.

Comment on lines +265 to +279
public int width() {
if (rotation % 2 == 0) {
return FilledMapItem.field_30907;
} else {
return FilledMapItem.field_30908;
}
}

public int height() {
if (rotation % 2 == 0) {
return FilledMapItem.field_30908;
} else {
return FilledMapItem.field_30907;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

These functions are unused.

Comment on lines +286 to +288
private record Vec2i(int x, int z) {

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

JOML already provides a Vector2i.

@xpple xpple added the enhancement New feature or request label Jul 3, 2023
@haykam821
Copy link
Collaborator

Is this pull request still maintained? The functionality can be extended in a later pull request to include #399 and #400.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants