squeek502 / Squake

Minecraft mod that adds Quake-style, Fortress Forever inspired movement to the game
The Unlicense
64 stars 19 forks source link

Unable to move below -128 with Squake loaded while using the Cubic Chunks mod #20

Closed loudcricketts closed 6 years ago

loudcricketts commented 6 years ago

When the Squake mod is installed, I am unable to move below Y-level -128 in a cubic chunks world. Whenever I reach that y-level I simply float around while being able to gain speed.

There are no crashes involved, and nothing shows up in console when I encounter this issue--regardless, here is a relevant console log from when I had both mods loaded together. https://gist.github.com/Megahaosguy/4a0f500e997d113adf59aabe10c9a179

Here is a short video demonstrating the issue: https://www.youtube.com/watch?v=iNfzwssVMMM

Mod list: Forge - 14.23.4.2705 Cubic Chunks - 1.12.2-0.0.852.0-SNAPSHOT-all_(FixHeightLangsCenter) Squake - mc1.12.2-1.0.5 Squeedometer - mc1.12.x-1.0.4 VanillaFix - 1.0.8-88

squeek502 commented 6 years ago

Interesting, thanks for letting me know. Will take a look.

loudcricketts commented 6 years ago

It also appears that exceeding y level 144 causes me to do very short jumps, and if I fall it is a slow descent until I pass 144. edit: corrected link https://www.youtube.com/watch?v=vb82ifj44zM

Thank you for viewing this!

squeek502 commented 6 years ago

(talking to myself for future reference)

Cubic Chunks seems to use its own posY, or override it in some way. Squake modifies EntityLivingBase.moveRelative and so does Cubic Chunks, so that's where the bug seems to be coming from (could be wrong about the method name though, might be wrong about this).

Relevant source code:

Barteks2x commented 6 years ago

I haven't touched the code in quite a long time but as I recall, the first one is for isBlockLoaded check to use entity Y coordinate. Since this is not considered a critical fix, it may fail and the mod still will run but with some functionality broken. Looking at the logs, it doesn't fail so the issue is likely somewhere else. And my best guess from the issue a a the Y coord a involved is that some part of your code is using isBlockLoaded with Y coordinate 0, so when cube at Y=0 is unloaded things stop working correctly.

Barteks2x commented 6 years ago

Ah I assumed this to be a coremod. Replacing the object is going to defeat that and I suspect this also causes incompatibility with sponge. If you are willing to make your mod compatible you could just use the Y coordinate in isBlockLoaded check even for vanilla as vanilla ignores it.

squeek502 commented 6 years ago

Squake is a coremod in 1.12 (previous versions offloaded that to PlayerAPI, but it's roughly the same in practice). The 1.12 transformer is here: https://github.com/squeek502/Squake/blob/1.12.2/java/squeek/quakemovement/ASMPlugin.java

In 1.12, for the travel method (previously moveEntityWithHeading), Squake inserts the equivalent of:

boolean cancelDefault = QuakeClientPlayer.moveEntityWithHeading(this, param1, param2, param3)
if (cancelDefault)
{
    return;
}

at the top of the method, and Squake returns there when it alters the movement (but it uses the default implementation for things like creative flying, riding an entity, ladder movement, etc).

Looks like this piece of code is the issue, where I re-implement some of Minecraft's code:

if (player.world.isRemote && (!player.world.isBlockLoaded(new BlockPos((int)player.posX, 0, (int)player.posZ)) || !player.world.getChunkFromBlockCoords(new BlockPos((int) player.posX, (int) player.posY, (int) player.posZ)).isLoaded()))
{
    if (player.posY > 0.0D)
    {
        player.motionY = -0.1D;
    }
    else
    {
        player.motionY = 0.0D;
    }
}
Barteks2x commented 6 years ago

I didn't see the 1.12 branch on mobile, blame github.

Just changing player.world.isBlockLoaded(new BlockPos((int)player.posX, 0, (int)player.posZ)) to player.world.isBlockLoaded(new BlockPos((int)player.posX, (int)player.posY, (int)player.posZ)) should work. In general I don't like when someone does what you do here - which is completely replace the implementation, but I also do it in a few places where it would be too complex to modify existing cod with mixin, and it's likely it would be complicated in your case, so this is probably the best option here.

Also, after using mixin for so long I almost completely forgot how unreadable ASM can be...

squeek502 commented 6 years ago

Sounds good, will make that change, but I'm unable to test it:

Could I get a quick overview of how Cubic Chunks is meant to be installed? I built it from source but am getting missing class org.spongepowered.asm.launch.MixinTweaker errors on launch. How is Sponge's mixin library meant to be included at runtime?

squeek502 commented 6 years ago

@Megahaosguy here's a version of Squake with the above fix if you want to test it: https://ryanliptak.com/misc/Squake-mc1.12.2-test.jar

Let me know if it fixes it for you.

Barteks2x commented 6 years ago

I will be able to test it myself within about 10 hours.

And you used the wrong jar to install cubic chunks. You need the -all jar, and make sure that the mixin annotation processor runs (do clean build if you aren't sure), otherwise refmap won't be generated and it will fail in obfuscated environment.

squeek502 commented 6 years ago

You need the -all jar

That was it, thanks. Confirmed fixed, will release a new version with the fix soon.