NoiseChunkGenerator should be in net.minecraft.world.gen #2057

OroArmor · 2021-02-13T04:41:19Z

java.lang.IllegalAccessError: class net.minecraft.world.gen.chunk.NoiseChunkGenerator tried to access protected method 'double net.minecraft.world.gen.StructureWeightSampler.getWeight(int, int, int)' (net.minecraft.world.gen.chunk.NoiseChunkGenerator and net.minecraft.world.gen.StructureWeightSampler are in unnamed module of loader 'app')

While I think the package for NoiseChunkGenerator is currently better, running Minecraft in a development environment causes this crash.

The text was updated successfully, but these errors were encountered:

liach · 2021-02-13T14:56:18Z

Hmm, seems the package visibility index from enigma has stopped working for long

liach · 2021-02-13T21:49:26Z

I doubt that the index works but the command line interface broke. Need to check.

modmuss50 · 2021-02-13T21:50:39Z

I did start looking at improving and fixing the mappings validator. Would be nice to try and get rid of these issues. But we cannot guarantee it

liach · 2021-02-13T22:07:57Z

Turns out check mappings was never broke. It was this that broke:
https://github.com/FabricMC/yarn/blame/21w06a/build.gradle#L396
Should be using intermediary jar

And this check mappings never worked since the intermediary update 2 years ago!

liach · 2021-02-13T22:09:42Z

Look at the current results:

ERROR: Must be in one package:
net/minecraft/client/gui/screen/ConnectScreen
net/minecraft/client/gui/screen/ConnectScreen$1
net/minecraft/client/gui/screen/CustomizeBuffetLevelScreen
net/minecraft/client/gui/screen/CustomizeBuffetLevelScreen$1
net/minecraft/client/gui/screen/CustomizeBuffetLevelScreen$BuffetBiomesListWidget
net/minecraft/client/gui/screen/CustomizeBuffetLevelScreen$BuffetBiomesListWidget$BuffetBiomeItem
net/minecraft/client/gui/screen/CustomizeFlatLevelScreen
net/minecraft/client/gui/screen/CustomizeFlatLevelScreen$1
net/minecraft/client/gui/screen/CustomizeFlatLevelScreen$SuperflatLayersListWidget
net/minecraft/client/gui/screen/CustomizeFlatLevelScreen$SuperflatLayersListWidget$SuperflatLayerItem
net/minecraft/client/gui/screen/DialogScreen
net/minecraft/client/gui/screen/DialogScreen$ChoiceButton
net/minecraft/client/gui/screen/PresetsScreen
net/minecraft/client/gui/screen/PresetsScreen$SuperflatPreset
net/minecraft/client/gui/screen/PresetsScreen$SuperflatPresetsListWidget
net/minecraft/client/gui/screen/PresetsScreen$SuperflatPresetsListWidget$SuperflatPresetEntry
net/minecraft/client/gui/screen/Screen
net/minecraft/client/gui/screen/option/LanguageOptionsScreen
net/minecraft/client/gui/screen/option/LanguageOptionsScreen$LanguageSelectionListWidget
net/minecraft/client/gui/screen/option/LanguageOptionsScreen$LanguageSelectionListWidget$LanguageEntry
net/minecraft/client/gui/screen/option/VideoOptionsScreen
ERROR: Must be in one package:
net/minecraft/world/gen/feature/DripstoneClusterFeature
net/minecraft/world/gen/feature/LargeDripstoneFeature
net/minecraft/world/gen/feature/LargeDripstoneFeature$1
net/minecraft/world/gen/feature/LargeDripstoneFeature$DripstoneGenerator
net/minecraft/world/gen/feature/LargeDripstoneFeature$WindModifier
net/minecraft/world/gen/feature/SmallDripstoneFeature
net/minecraft/world/gen/feature/util/DripstoneHelper
ERROR: Must be in one package:
net/minecraft/client/util/math/AffineTransformation
net/minecraft/client/util/math/Vector4f
net/minecraft/util/math/DirectionTransformation
net/minecraft/util/math/DirectionTransformation$1
net/minecraft/util/math/Matrix3f
net/minecraft/util/math/Matrix4f
net/minecraft/util/math/Vec3f
ERROR: Must be in one package:
net/minecraft/util/dynamic/RegistryElementCodec
net/minecraft/util/dynamic/RegistryOps
net/minecraft/util/dynamic/RegistryOps$1
net/minecraft/util/dynamic/RegistryOps$EntryLoader
net/minecraft/util/dynamic/RegistryOps$EntryLoader$1
net/minecraft/util/dynamic/RegistryOps$EntryLoader$Impl
net/minecraft/util/dynamic/RegistryOps$ValueHolder
net/minecraft/util/dynamic/RegistryReadingOps
net/minecraft/util/registry/RegistryLookupCodec
ERROR: Must be in one package:
net/minecraft/world/gen/AquiferSampler
net/minecraft/world/gen/StructureWeightSampler
net/minecraft/world/gen/chunk/ChunkGeneratorSettings
net/minecraft/world/gen/chunk/NoiseChunkGenerator
ERROR: Must be in one package:
net/minecraft/world/chunk/EntityChunkDataAccess
net/minecraft/world/storage/RegionBasedStorage
net/minecraft/world/storage/SerializingRegionBasedStorage
net/minecraft/world/storage/StorageIoWorker
net/minecraft/world/storage/StorageIoWorker$Priority
net/minecraft/world/storage/StorageIoWorker$Result
net/minecraft/world/storage/VersionedChunkStorage
ERROR: Must be in one package:
net/minecraft/entity/EyeOfEnderEntity
net/minecraft/entity/projectile/ExplosiveProjectileEntity
net/minecraft/entity/projectile/FireworkRocketEntity
net/minecraft/entity/projectile/FishingBobberEntity
net/minecraft/entity/projectile/FishingBobberEntity$1
net/minecraft/entity/projectile/FishingBobberEntity$PositionType
net/minecraft/entity/projectile/FishingBobberEntity$State
net/minecraft/entity/projectile/LlamaSpitEntity
net/minecraft/entity/projectile/PersistentProjectileEntity
net/minecraft/entity/projectile/PersistentProjectileEntity$PickupPermission
net/minecraft/entity/projectile/ProjectileEntity
net/minecraft/entity/projectile/ShulkerBulletEntity
net/minecraft/entity/projectile/thrown/ThrownEntity
ERROR: Must be in one package:
net/minecraft/client/Keyboard
net/minecraft/client/Keyboard$1
net/minecraft/client/MinecraftClient
net/minecraft/client/MinecraftClient$1
net/minecraft/client/MinecraftClient$IntegratedResourceManager
net/minecraft/client/MinecraftClient$WorldLoadAction
net/minecraft/client/Mouse
net/minecraft/client/option/FullscreenOption
net/minecraft/client/option/Option
net/minecraft/client/option/Option$1
ERROR: Must be in one package:
net/minecraft/entity/ai/brain/task/AdmireItemTask
net/minecraft/entity/ai/brain/task/HuntFinishTask
net/minecraft/entity/ai/brain/task/HuntHoglinTask
net/minecraft/entity/ai/brain/task/RemoveOffHandItemTask
net/minecraft/entity/mob/AbstractPiglinEntity
net/minecraft/entity/mob/PiglinBrain
net/minecraft/entity/mob/PiglinBruteBrain
net/minecraft/entity/mob/PiglinBruteEntity
net/minecraft/entity/mob/PiglinEntity
ERROR: Must be in one package:
net/minecraft/client/gui/screen/world/CreateWorldScreen
net/minecraft/client/gui/screen/world/CreateWorldScreen$1
net/minecraft/client/gui/screen/world/CreateWorldScreen$Mode
net/minecraft/client/gui/screen/world/CreateWorldScreen$WorldCreationException
net/minecraft/client/gui/screen/world/MoreOptionsDialog
net/minecraft/client/world/GeneratorType
net/minecraft/client/world/GeneratorType$1
net/minecraft/client/world/GeneratorType$2
net/minecraft/client/world/GeneratorType$3
net/minecraft/client/world/GeneratorType$4
net/minecraft/client/world/GeneratorType$5
net/minecraft/client/world/GeneratorType$6
net/minecraft/client/world/GeneratorType$7
net/minecraft/client/world/GeneratorType$8
net/minecraft/client/world/GeneratorType$ScreenProvider
ERROR: Must be in one package:
net/minecraft/client/model/ModelCuboidData
net/minecraft/client/model/ModelPartBuilder
net/minecraft/client/util/math/Dilation
ERROR: Must be in one package:
net/minecraft/screen/ScreenHandler
net/minecraft/screen/slot/Slot
ERROR: Must be in one package:
net/minecraft/screen/AbstractFurnaceScreenHandler
net/minecraft/screen/slot/FurnaceFuelSlot

modmuss50 · 2021-02-13T22:11:59Z

Ouch, thats quite a lot there. We could just take the stance of not caring in favor of better mappings? Worth discussing

liach · 2021-02-13T22:18:07Z

Imo we will just fix it. But I will probably patch this task to work like javadoc (our javadoc does not fail if the doclint errors). So we can probably catch that illegal state exception thrown and just print the error message, or only fail when we are not on ci

liach · 2021-02-13T22:32:21Z

Related snippet:

ERROR: Must be in one package:
net/minecraft/world/gen/AquiferSampler
net/minecraft/world/gen/StructureWeightSampler
net/minecraft/world/gen/chunk/ChunkGeneratorSettings
net/minecraft/world/gen/chunk/NoiseChunkGenerator

liach · 2021-02-13T23:00:23Z

Looking back, some stuff probably had better be exposed than left package private or protected.

From #128 (comment):

They're a hint, sure, but compatibility with Mojang's internal mappings was never a goal.

If classes like Screen, ProjectileEntity etc (high traffic public api-like classes) do have package private stuff reserved for their peers, I guess either way mods will seek to open up access because their api-like nature. But in most other cases we see here, I'd say keeping them in same package is somehow more beneficial.

Juuxel · 2021-02-13T23:19:47Z

IMO we should keep the package-private friends together in the same packages. It might result in worse mappings in some places, but they're also more correct behaviour-wise.

liach · 2021-02-14T04:26:46Z

A few interesting things stand out. For example, in that message above it says ThrownEntity must be in same package as ProjectileEntity, yet I don't see why; ThrownEntity though uses one protected method from ProjectileEntity in its lambda construction (which javac afaik usually generates a bridge method for) even if jdk's lambda meta factory can totally handle that. Just putting a reminder for myself to check if package visibility index from enigma needs an update.

liach · 2021-02-14T18:20:41Z

Turns out that brainfart enough, ProjectileEntity's constructor is package private. That's why...
So the warnings from visibility index look to be all valid, such as when javac doesn't generate bridge methods. Now we need to move on to fix those problems...

as required by visibility Fixes FabricMC#2057 Signed-off-by: liach <liach@users.noreply.github.com>

Chocohead · 2021-02-14T22:01:34Z

I would argue the opposite, given we fix the behaviour either way there's no advantage to worse mappings just because Mojang happen to use a particular package structure. Not least that you don't even need Loader's make everything public at dev runtime hack anymore, Tiny Remapper can do it directly in the places where it is needed.

YanisBft · 2021-02-14T23:40:51Z

I agree with Chocohead, I'm not in favor of moving classes around just to respect Mojang's package structure. We need to fix the issues elsewhere in the toolchain

liach · 2021-02-15T00:40:04Z

I would argue the opposite, given we fix the behaviour either way there's no advantage to worse mappings just because Mojang happen to use a particular package structure. Not least that you don't even need Loader's make everything public at dev runtime hack anymore, Tiny Remapper can do it directly in the places where it is needed.

Hmm, so we need to declare that we want to fix access instead at https://github.com/FabricMC/fabric-loom/blob/57c9a8f3209121f22e84b8b01b64f1093256f1d3/src/main/java/net/fabricmc/loom/configuration/providers/minecraft/MinecraftMappedProvider.java#L129-L136 and

yarn/build.gradle

Lines 826 to 830 in 629b96e

    
           def remapper = TinyRemapper.newRemapper() 
        
           		.withMappings(TinyUtils.createTinyMappingProvider(mappings.toPath(), from, to)) 
        
           		.renameInvalidLocals(true) 
        
           		.rebuildSourceFilenames(true) 
        
           		.build()

The loom refactor proposal is good that we can actually utilize split loom to reduce duplicate code between yarn and loom

Needs a loom pr as well. The loom pr can properly fix the problem in dev env. Signed-off-by: liach <liach@users.noreply.github.com>

Properly fix FabricMC/yarn#2057 Signed-off-by: liach <liach@users.noreply.github.com>

liach · 2021-03-28T19:47:30Z

Closing this now.

For people wondering, the solution is always to run the named (yarn) mapped jar/bytecode through fabric loader. This includes calling data generators from your mods' dev env!

liach added the bug Fixes or discusses a bug within the mappings label Feb 13, 2021

liach added the refactor A PR that renames existing names. label Feb 13, 2021

liach added the discussion label Feb 13, 2021

liach mentioned this issue Feb 13, 2021

Make mapping check work again #2066

Merged

liach self-assigned this Feb 14, 2021

liach added the toolchain label Feb 14, 2021

liach added a commit to liach/yarn that referenced this issue Feb 14, 2021

Move aquifer and structure weight samplers to chunk gen package

f498af5

as required by visibility Fixes FabricMC#2057 Signed-off-by: liach <liach@users.noreply.github.com>

This was referenced Feb 14, 2021

Move aquifer and structure weight samplers to chunk gen package #2069

Closed

Move some piglin stuff to entity/mob/piglin to prevent package #2077

Closed

liach mentioned this issue Feb 15, 2021

Nuke option screen package due to package visibility requirements #2072

Closed

liach added a commit to liach/yarn that referenced this issue Feb 15, 2021

Properly fix FabricMC#2057

57330f7

Needs a loom pr as well. The loom pr can properly fix the problem in dev env. Signed-off-by: liach <liach@users.noreply.github.com>

liach mentioned this issue Feb 15, 2021

Properly fix #2057 by asking tiny remapper to patch package access #2078

Closed

liach added a commit to liachmodded/fabric-loom that referenced this issue Feb 15, 2021

Make tiny remapper used by loom fix package access for named namespace

3c239d5

Properly fix FabricMC/yarn#2057 Signed-off-by: liach <liach@users.noreply.github.com>

This was referenced Feb 15, 2021

Make tiny remapper used by loom fix package access for named namespace FabricMC/fabric-loom#352

Closed

In ide, minecraft classes should be loaded with knot than java's default class loader FabricMC/fabric-loom#353

Closed

This was referenced Feb 16, 2021

Tiny v2 spec: new standard property - check-package-access FabricMC/tiny-remapper#46

Closed

Mojang package structure #649

Closed

liach closed this as completed Mar 28, 2021

liach mentioned this issue Aug 3, 2021

method_7672 of class_1735 has protected access but is accessed in a public way by class_1703 #2622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NoiseChunkGenerator should be in net.minecraft.world.gen #2057

NoiseChunkGenerator should be in net.minecraft.world.gen #2057

OroArmor commented Feb 13, 2021

liach commented Feb 13, 2021

liach commented Feb 13, 2021

modmuss50 commented Feb 13, 2021

liach commented Feb 13, 2021 •

edited

liach commented Feb 13, 2021

modmuss50 commented Feb 13, 2021

liach commented Feb 13, 2021

liach commented Feb 13, 2021

liach commented Feb 13, 2021

Juuxel commented Feb 13, 2021

liach commented Feb 14, 2021

liach commented Feb 14, 2021

Chocohead commented Feb 14, 2021

YanisBft commented Feb 14, 2021

liach commented Feb 15, 2021

liach commented Mar 28, 2021

NoiseChunkGenerator should be in net.minecraft.world.gen #2057

NoiseChunkGenerator should be in net.minecraft.world.gen #2057

Comments

OroArmor commented Feb 13, 2021

liach commented Feb 13, 2021

liach commented Feb 13, 2021

modmuss50 commented Feb 13, 2021

liach commented Feb 13, 2021 • edited

liach commented Feb 13, 2021

modmuss50 commented Feb 13, 2021

liach commented Feb 13, 2021

liach commented Feb 13, 2021

liach commented Feb 13, 2021

Juuxel commented Feb 13, 2021

liach commented Feb 14, 2021

liach commented Feb 14, 2021

Chocohead commented Feb 14, 2021

YanisBft commented Feb 14, 2021

liach commented Feb 15, 2021

liach commented Mar 28, 2021

liach commented Feb 13, 2021 •

edited