micdoodle8 / MicdoodleCore

Includes ASM class transformers and core classes for my Minecraft mods.
Other
15 stars 18 forks source link

Specify FMLAT for access transformer in MANIFEST.MF, instead of using custom Access Transformer class (for better dev environment compatibility) #9

Open Barteks2x opened 7 years ago

Barteks2x commented 7 years ago

I tried to run galacticraft in dev environment, but I found that it crashes as soon as I run /dimensiontp, it was caused by a difference in used mappings. Galacticraft currently applies special deobf access transformer on it's own, which seems to be useless now. After I removed the deobf AT, moved the existing one manually to META-INF and specified FMLAT entry in MANIFEST.MF the mod ran just fine.

I can try to submit PR for that if needed.

radfast commented 7 years ago

A PR would be much appreciated.

Barteks2x commented 7 years ago

I also found an issue with the class transformer when used in dev environment... it uses the mappings it expects instead of what is actually there. Which crashes when calling Chunk#populate() for me

I would suggest to use System.getProperty("net.minecraftforge.gradle.GradleStart.srg.srg-mcp") to get actual mappings file.

I'm not even sure how to workaround it

Barteks2x commented 7 years ago

I probably won't make a PR anytime soon, as after some initial testing I figured out that galacticraft just won't be compatible with my mod without possibly major internal changes, so I don't need it working in dev environment now. I will probably still make the PR when I'm less busy.

radfast commented 7 years ago

Thanks for this, we are aiming to make a dev version which works for people developing other mods. But our de-obf version does target SRG mappings for a specific Forge version as specified in our build.gradle, so if you're using a different Forge / SRG version in your mod's workspace that is going to give rise to problems. It will be very time-consuming to re-write the ClassTransformer to get the mappings, and would risk introducing new bugs, but we should probably do that long term - I'll talk to micdoodle8 and see what we can do here.

Which mod are you looking for compatibility with? McWorldGenLoop?

In principle we want to be compatible with all other mods if possible - especially mods which make sense in a Galacticraft universe - and we are happy to add compatibility features in our mod, but we would need you to specify exactly what is needed.

I am extremely interested in looping worldgen. Galacticraft currently maps the Overworld from approximate x z coords (-12000, -7000) -> (12000, 7000) and generates an image with that worldmap - you can find the image client-side in .minecraft/assets, but you will see it there only after the game world has been running for ~30-60 minutes. It would be nice if that map looped. In future that map will maybe form the basis of space views of the Overworld.

Barteks2x commented 7 years ago

I'm trying to get compatibility with something way more extreme than worldgen loop (which is on github just because someone asked me to push it) - I'm talking about the Cubic Chunks mod.

The main problem is hardcoded height limit, other problem is a completely messed up way Galacticraft transfers players between dimensions.

radfast commented 7 years ago

a completely messed up way Galacticraft transfers players between dimensions.

lol thank you! we almost exactly replicate transfers to and from the Nether, only without the 8x change in x, z co-ordinates.

hard-coded height limit is something which could relatively easily be changed or made configurable, though because our mod makes extensive use of height above y = 256 there's a more fundamental question of what exactly is needed to make the mods compatible.

Above y = 256:

These rocket mechanics are not going to make any sense if the world is built up to much larger heights.

Barteks2x commented 7 years ago

If GC has API to change these values for rocket machanics, I can adjust these values based on terrain generator settings. It could also be based on height above the surface, so players can't "cheat" by buildint tall structures, and it won't be so confusing when buildint tall structures. As for height limit - in some recent forge versions my PR that makes World#isValid and World#isOutsideBuildHeight public got accepted, you could use these methods instead.

As for player dimension transfer - it's NOT the same as vanilla. Nowhere near. I will make separate issue for that because I belive it should be fixed, even if it just happens to work with vanilla (and I'm still puzzled about why it even works). Vanilla dimension transfer works fine with my mod, to make GC work I had to make it much more forgiving of incorrect usage.

radfast commented 7 years ago

I'm not too interested in isValid() or isOutsideBuildHeight() calls as most of our height-related code is to do with entities and rendering, not block placement.

Seems like the best approach may be something in our API which calls for the "local" terrain height - maybe an event - and your mod can listen for that and return a different local terrain height. All our hard-coded heights can then be a delta from the local terrain height. No 100% guarantee that we will do this but seems like the best way.


Please do give specific details of what you believe needs changing in player dimension transfer. We will be grateful for any improvements.

Barteks2x commented 7 years ago

I know for sure that placing torches below y=0 crashes the game because of hardcoded height check in BlockVec3#getBlockState_noChunkLoad (and a few other methods there do the same), that's where isValid could be used.

As for player transfer - still writing up issue on that, and investigating why there is no message spam in log file with vanilla, that I would expect. But for that I need to set up galacticraft dev environment and add my mod to it instead of adding GC to my dev environment (my mod uses mixin which should handle different mappings)

Barteks2x commented 7 years ago

I'm kind of stuck trying to setup galacticraft dev environment, is there any information on how to do it? It's one of the few mods where gradle build doesn't work after running setupDecompWorkspace

radfast commented 7 years ago

https://wiki.micdoodle8.com/wiki/GC3_API#Full_Galacticraft_Sources

Guide was written for 1.7.10 so you might have to adapt it slightly to 1.8+, I'm not sure whether you are looking for 1.8.9, 1.10.2 or 1.11.2

Key steps:

  1. identify from our build.gradle the precise Forge and SRG mappings which we are targeting and get the Forge MDK for that
  2. place all necessary class transformers into your new Forge workspace where ForgeGradle can find them - the key one is micdoodlecore.at but also look at the dependencies resources in Galacticraft repository for whichever MC version
  3. run setupdecompworkspace
  4. then you'll need all the sources from this repository + the Galacticraft repository - in Eclipse some people just use the Galacticraft repository sources and have MicdoodleCore as a library, but I think IntelliJ will give you a circular reference error, so you need both sets of sources in the same workspace with IntelliJ

You may not find our set up ideal - more accurately, it may not be the same as your mod or other mods - but it works if you follow those steps and I'd be fairly opposed to changing it for reasons

radfast commented 7 years ago

I know for sure that placing torches below y=0 crashes the game because of hardcoded height check in BlockVec3#getBlockState_noChunkLoad (and a few other methods there do the same), that's where isValid could be used.

ok, that's something we can probably fix easily

radfast commented 7 years ago

As for player transfer - still writing up issue on that, and investigating why there is no message spam in log file with vanilla, that I would expect. But for that I need to set up galacticraft dev environment and add my mod to it instead of adding GC to my dev environment (my mod uses mixin which should handle different mappings)

We are interested in this issue. Galacticraft's player transfers currently work error-free with vanilla/Forge versions 1.7.2 through to 1.11.2 including Sponge servers for MC1.10.2 and 1.11.2. Though actually we do have a minor issue with Sponge builds later than 2244, something connected with their CauseTracker blocks certain server-client notifications (eg. entity spawn packets) after a player dimension change. I'm curious to see if what you're going to show me puts some new light on that...

Barteks2x commented 7 years ago

Here when it says "Forge workspace" does it mean forge development workspace, or mod development workspace (MDK)? (I've never actually tried adding mod source to actual forge dev environment, but adding ATs doesn't look like something I would do with the normal MDK)

radfast commented 7 years ago

You have a folder which you unpacked the MDK into. That is your workspace. Copy the ATs into the /src folder, I recommend put them all /src/main/resources. Then run setupdecompworkspace, you should notice that after the startup messages ForgeGradle's console output will now report that it found the ATs.

You can run setupdecompworkspace like this even after you already ran setupdecompworkspace before in the same workspace, it will do the whole series of tasks again but now with the ATs. Or you can create a new workspace, that's up to you.

I am not aware of any other way to have ForgeGradle take account of the ATs, the Forge system is not so flexible.

thiakil commented 6 years ago

I am not aware of any other way to have ForgeGradle take account of the ATs, the Forge system is not so flexible.

If you have a proper gradle setup forgegradle is perfectly flexible. You seem to have everything as unpacked sources instead of using maven like everyone else.

A proper setup means your long and confusing instructions are not required at all. Mods should only require

to have it build