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.

Sign up for GitHub

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

Jump to bottom

1.21.2 #305

Merged
merged 7 commits into from
Nov 14, 2024
Merged

1.21.2 #305

merged 7 commits into from
Nov 14, 2024

Conversation

arnokeesman
Copy link
Contributor

there's a deprecation warning on HeightFindingStrategy.java#58
other than that, it seems fine, the server starts, haven't been able to test more than that

actions refused to run, so updated those

John-Paul-R
John-Paul-R reviewed Oct 26, 2024
View reviewed changes
src/main/java/com/fibermc/essentialcommands/commands/utility/SuicideCommand.java Outdated
@@ -21,7 +21,7 @@ public int run(CommandContext<ServerCommandSource> context) throws CommandSyntax
return 0;
}

player.kill();
player.kill(player.server.getWorld(player.getWorld().getRegistryKey()));
Copy link
Owner

Choose a reason for hiding this comment

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

lmao

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah so apparently players can now not be in a world

Copy link
Owner

Choose a reason for hiding this comment

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

Mojang, what? (Entity.java)

image

For now I'll just recommend doing player.getServerWorld() since we know this is a ServerPlayerEntity already.

Copy link
Owner

@John-Paul-R John-Paul-R Oct 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

players can now not be in a world

I somehow read the as the opposite of what you wrote. If that's actually a thing, maybe steal what /kill does - source.getContext().getWorld() (E: Wait, how does that work for console command-sender?!)

John-Paul-R
John-Paul-R reviewed Oct 26, 2024
View reviewed changes
src/main/java/com/fibermc/essentialcommands/mixin/PlayerManagerMixin.java Outdated
@@ -122,7 +120,7 @@ public void onRespawnPlayer_forResawnLocationOverwrite(
Vec3d.ZERO,
0,
0,
false,
PositionFlag.ROT,
Copy link
Owner

Choose a reason for hiding this comment

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

since this is a respawn, I think we should just have nothing be "relative" to current position, so I'd just drop this parameter.

src/main/java/com/fibermc/essentialcommands/mixin/ServerPlayNetworkHandlerMixin.java
@@ -15,11 +16,13 @@
@Mixin(ServerPlayNetworkHandler.class)
public class ServerPlayNetworkHandlerMixin {

@Unique
Copy link
Owner

Choose a reason for hiding this comment

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

nice, +1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

blame the IDE, I wouldn't've caught it

src/main/java/com/fibermc/essentialcommands/mixin/ServerPlayerEntityMixin.java Outdated
Comment on lines 40 to 42
@Shadow
public abstract boolean damage(ServerWorld world, DamageSource source, float amount);

Copy link
Owner

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doubt past Arno would have a better answer for this, but present Arno really doesn't have a clue, doesn't even seem to have any usages, consider it gone

src/main/java/com/fibermc/essentialcommands/mixin/TeleportCommandMixin.java
@@ -35,9 +35,8 @@ private static void execute(
@Coerce Object facingLocation,
CallbackInfo ci
) throws CommandSyntaxException {
if (target instanceof ServerPlayerEntity) {
if (target instanceof ServerPlayerEntity targetPlayer) {
Copy link
Owner

Choose a reason for hiding this comment

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

Evidently I am living in the past. Did not know instanceof could be a pattern-matching declaration operator-thing. Cool!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IntelliJ starts complaining when I don't do it for some reason

src/main/java/com/fibermc/essentialcommands/teleportation/PlayerTeleporter.java Outdated
@@ -65,7 +66,8 @@ private static void execTeleport(ServerPlayerEntity playerEntity, MinecraftLocat
playerEntity.teleport(
targetWorld,
dest.pos().x, dest.pos().y, dest.pos().z,
dest.headYaw(), dest.pitch()
PositionFlag.ROT, dest.headYaw(), dest.pitch(),
Copy link
Owner

Choose a reason for hiding this comment

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

Because we're setting the yaw and pitch explicitly from the dest, I don't think we want any relative positioning on ROT here. (I thing and empty set (Set.of()) would be what we want, probably?)

(Though this makes me realize an optional pitch/headYaw on MinecraftLocation may be useful, but that is a consideration for another time :P)

src/main/java/com/fibermc/essentialcommands/commands/utility/FeedCommand.java
Comment on lines +28 to +29
// hungerManager.setExhaustion(0);
// idk, you ca only add to it now
Copy link
Owner

Choose a reason for hiding this comment

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

good call out, will address in #306

@John-Paul-R
Copy link
Owner

Thanks for doing this! Is a massive help

(btw if you'd prefer I just push changes to your branch for this ^ style of review, just lmk)

John-Paul-R
John-Paul-R reviewed Oct 26, 2024
View reviewed changes
build.gradle Outdated
@@ -1,6 +1,6 @@
plugins {
id 'groovy'
id 'fabric-loom' version '1.6-SNAPSHOT'
id 'fabric-loom' version '1.8-SNAPSHOT'
Copy link
Owner

Choose a reason for hiding this comment

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

I can't seem to get genSources to work with fabric-loom @ 1.8, so I'd recommend downgrading to 1.7 unless it breaks something else (doesn't appear to?).

Related discord messages

Suggested change
id 'fabric-loom' version '1.8-SNAPSHOT'
id 'fabric-loom' version '1.7-SNAPSHOT'

arnokeesman
arnokeesman commented Oct 30, 2024
View reviewed changes
build.gradle
@@ -1,6 +1,6 @@
plugins {
id 'groovy'
id 'fabric-loom' version '1.8-SNAPSHOT'
id 'fabric-loom' version '1.7-SNAPSHOT'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔

Copy link
Owner

Choose a reason for hiding this comment

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

(see the comment above this one)

This is visible right? (I've managed to have my review comments not posted in the past... lol)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

depends on which commits are selected it seems, it does show at that link, just not on this link, probably because the comment is on a line that doesn't exist anymore

@arnokeesman arnokeesman force-pushed the 1.21.2 branch 3 times, most recently from f4da03a to 4a48aea Compare November 14, 2024 20:44
@arnokeesman
Copy link
Contributor Author

damn, github just has a button to rebase and force-push now
image
ok that broke the build somehow, thanks for the button, guess we're back to git commands
welp, at least it builds on your repo now, still fails on mine

anyway, last thing we might want to change is dropping support for 21.2 and going straight for 21.3

@John-Paul-R
Copy link
Owner

Thanks! I'll merge this but publish for 1.21.3.

@John-Paul-R John-Paul-R merged commit 7ea9845 into John-Paul-R:1.21.x Nov 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@John-Paul-R John-Paul-R John-Paul-R left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@arnokeesman @John-Paul-R