ornithemc / feather-mappings Goto Github PK

C_5577922 and C_7492943, currently named Box and StructureBox respectively, are two classes used to define axis aligned bounding boxes. While Box is used in a variety of contexts like entity and block shapes, collisions, and rendering, StructureBox on the other hand is used exclusively for structure bounding boxes.

For reference, modern mojmaps uses AABB and BoundingBox respectively for these classes.

Should we adopt the mojmaps convention? Stick with our current names? Or do something in between?

Minecraft Version
Anything between 1.14.4 and down to Beta, probably further but I haven't checked Alpha or below.

Type
Method

Describe where the element is
These are the methods in 1.13.2, the names and descriptors are different in older versions.

1. World.onBlockChanged(BlockPos, BlockState, BlockState, int)V
2. World.onBlockChanged(BlockPos, Block)V

Description of the element

1. Notify all world event listeners that the block has changed from the first to the second given state, and with which flags. This method is called from setBlockState on both the client and server side, depending on the given flags. On the server, the server world event listener notifies the chunk map, which will send the block change to the client. On the client, this method notifies the world renderer of the block change.
2. Notify neighboring blocks through neighborChanged updates (i.e. block updates) that this block has changed . This method is called from setBlockState if and only if the 0x1 flag is set and the world is server side.

Current name

1. m_0384893 -> onBlockChanged
2. m_5170064 -> onBlockChanged

Expected name
I propose we rename the first method to sendBlockChanged or notifyBlockChanged. Perhaps "notify" makes the most sense considering "send" is not correct for the client side.

Type
Class

Describe where the element is
net/minecraft/client/render/texture/

Description of the element
In 1.5 and above, it describes a single texture sprite, which the game then stitches together into an atlas. However, in prior versions this class represents the entire atlas.

Expected name
TextureAtlas

Minecraft Version
beta 1.7.3

Type
Field

Describe where the element is
Entity.java

Description of the element
Sets the minimum brightness of an entity, used to make the player model in the inventory render fullbright, also acts as a fallback brightness for entities which are in unloaded areas.

Expected name
minimumBrightness

Additional Context

We have had some discussions on Discord about naming and style conventions, and the numerous inconsistencies in the current mappings. The style guide should be updated to include the things we discussed and then the mappings should be updated to make sure it follows these guidelines correctly.

Suggested changes/additions:

• Handling of anonymous classes - before 1.8.2-pre5 anonymous and inner classes were stripped from their parent class. These classes should be treated as any other regular class in terms of naming and packaging, but a note should be included in the java docs to state that it is an anonymous class and link to its parent class.
• Use random, not rand.
• Use world, not level.
• Use parentBlockEntity for block entities owned by screens.
• Use client for MinecraftClient fields.
• Use connection for ClientConnection fields
• Use TEXTURE for static final Identifier fields which store the main/default texture used for a model/screen/similar thing.
• Use <PREFIX>_TEXTURE for static final Identifier fields which store non-default/-main textures or textures which would not make sense to name with the above convention (for example INVULNERABLE_TEXTURE in WitherEntityRenderer or SHADOW_TEXTURE in EntityRenderer).
• Use MODEL for static final Model fields which store the main/default model of a class.
• Use <PREFIX>_MODEL for static final Model fields which store extra/alternative models for a class, or for all models in a class where there is no clear "main" model.
• Do not use the Abstract prefix when naming abstract classes if possible.
• Use dir for Direction objects (1.8+) or ints representing any of the six possible directions.
  • Use face when referring to a specific side of a block.
• Use facing for ints representing cardinal directions.
• When a class contains a single instance of itself as a static field, name it INSTANCE (all capitalized, even if the field is not final).
• For NBT related methods:
  • Use toNbt for non static methods that convert the object to NBT.
  • Use fromNbt for static methods that take in NBT and create an object from it.
  • Use writeNbt for non static methods that take in NBT and write data to it.
  • Use readNbt for non static methods that take in NBT and read data from it.
• Use buffer for ByteBuf and PacketByteBuf parameters.
• Use font for TextRenderer fields/parameters.

Feel free to suggest other changes or additions in the comments and I will keep this post updated with the changes we agree upon.

In 1.8, the method Block#updateShape is overloaded, and does two related, but distinct things:

The method with the signature BlockState updateShape(BlockState state, WorldView world, BlockPos pos) updates the properties of the blockstate. For most blocks, it does nothing, but for blocks that have blockstates with "virtual properties", it resolves these properties by looking at the neighboring blocks using the WorldView parameter. This method is used for 4 things:

1. It is used to determine the correct BakedModel to use for the fully resolved virtual blockstate in rendering
2. It is used to display the correct debug info in the F3 screen
3. It is used in ParticleManager#addBlockMiningParticles - however, this call is redundant and does nothing, as no block mining particles are affected by any virtual properties
4. It is used in DoublePlantBlock#getVariant/canBeReplaced to resolve the VARIANT property, because for the upper half of double plant blocks, VARIANT is a virtual property that depends on the blockstate below it. This usage is technically also superfluous, because the virtual property of the block does not actually have to be updated.

The method with the signature void updateShape(WorldView world, BlockPos pos) instead sets the boundaries of the block by calling setShape. The usages of this method are harder to categorize, but the major usages are as follows:

1. The method Block#rayTrace calls it to update the boundaries of the block for raytracing a HitResult. Blocks with multiple hitboxes, such as stairs (but not hoppers or cauldrons), override Block#rayTrace, calling the super method multiple times, also overriding updateShape to update the hitbox between multiple calls.
2. Some blocks override Block#getOutlineShape to call this method.

A proposed renaming of these methods:

• The first method should be renamed to resolveVirtualProperties - I feel this name is quite self explanatory to anybody who understands what virtual properties are
• The second method could be renamed to updateHitboxBounds, or stay as is.

Minecraft Version
Beta 1.7.3

Names of the field
f_9892467 in Dimension.java

Additional Context
Every place in the code where this is used is stuff that the Nether effects so I'm 99% sure this is nether related, stuff like determining if the bed should blow up or if the compass and clock should spin

Minecraft Version
1.12.2 (probably starts around 1.9, possibly earlier)

Type
Method

Describe where the element is
getName()Ljava/lang/String; in net/minecraft/world/chunk/ChunkSource

Description of the element
It returns the debug info for that chunk source.

Current name
getName

Expected name
getDebugInfo

Currently, the Feather buildscript uses Gradle to execute all of its tasks, from running the main feather task, mapping generation, and name propagation, to downloading necessary libraries and publishing the mappings to the Ornithe maven repository. This has mostly worked, but Gradle has a tendency to time out or reset its connection randomly for no discernable reason. This was previously a really significant issue when we would publish every version in the same task sequentially, but #134 has significantly improved that situation by running every publish task in parallel, publishing up to 20 versions at a time concurrently.

This solved the issue of having to restart the whole publish task for every version if one of the versions failed, but the second time the new publish workflow was run, many versions failed to publish because the publish tasks would try to upload an artifact version that was already on the repository, because the maven-metadata.xml file didn't have those versions listed in them. We believe the concurrent nature of the workflow is causing a race condition with reading and writing the maven-metadata.xml file, preventing it from getting accurately updated for every version.

When thinking of ways to resolve the first issue, I had the idea of transferring most, if not all, of Gradle's workload to Pure Java Code; I shelved this idea when @Kahzerx refactored the workflow, but this race condition issue is making me consider bringing up this ussie again.

There are a couple things to consider:

• What are all the tasks that are part of the current Gradle buildscript that we would need to translate to Java? Are there tasks we can get rid of?
• Is there any new functionality that would be nice to add to a potential Java buildscript that was unfeasible to have when we used Gradle?
  • This might be a way that we could publish all the versions without updating the maven-metadata.xml file, until the very end.
• Where are we storing the cache for these tasks, like version jars and the like? Should we try to use the same cache folder that our current Gradle buildscript uses? Should we use a different location?
• Related to the previous point, should we use Gradle as a sort of "bootstrap" to run the core Java code part of this potential buildscript? Or should we ship with a Pure Java Code bootstrap main class that we compile on the fly that runs the rest of the buildscript?

I'm getting to the point where I've written so much that I'm losing track of my thoughts, so I'll stop here, I might edit this more later. Please feel free to share your thoughts about this.

The idea is to remove the insertAutoGeneratedEnumMappings task. This task currently runs stitchs CommandProposeV2FieldNames to generate names from the non-stripped enum variants (at least that's what I think it does). Removing this would require us to map these enum variants ourself. This is not hard, we already do this (I think).
In for example 1.12.2 the task only replaced 16, but detected 2184 "interesting names".
In 12w30e-client it replaced 0 names, and detected 130 "interesting names".

Therefore I think we should just remove that task and simpify the building process.

In the image you can see what would be replaced: the two red arrows to and the one from the insertAutoGeneratedEnumMappings task would be replaced with the green one that directly connects buildFeatherTiny with v2UnmergedFeatherJar.

Note: I plan to open a PR in the future containing that graph.

In terms of code changes:
The task to be removed is:

This issue is created to discuss how much this would break, since I do not currently know how dependant the mappings are on this feature.

for the future PR: (just using this as notes for later on)

• Remove the insertAutoGeneratedEnumMappings gradle task and instead map these directly; see #142
  I do not yet know the impacts of this, see the issue for this.
  ... maybe there's more that could be cleaned up:
• Why do we use tiny v1 in so many places? We should just switch all to tiny v2 and generate tiny v1 from them.

Type
Class

Describe where the element is
net/minecraft/client/render/model/entity/EnderDragonRenderer
net/minecraft/client/render/model/entity/EndermanRenderer

Expected position
net/minecraft/client/render/entity/EnderDragonRenderer
net/minecraft/client/render/entity/EndermanRenderer

Additional Context
net/minecraft/client/render/model is reserved for models, renderers, especially entity renderers, should be in net/minecraft/client/render/entity

This file is very much outdated and should be updated to list the maintainers of Feather.

Type
Field

Describe where the element is
Lnet/minecraft/client/Minecraft;paused:Z

Description of the element
Stores whether the game is running in an applet

Expected name
appletMode

Additional Context
It is unknown to me how this mapping affects other versions

Some people have brought up the topic of making a Github organization to better organize all the repositories associated with the Ornithe project, but there are a couple things that have to be ironed out at the time repository transfer occurs in order to reduce any possible issues that could arise from this, and I'd want to hear some opinions from others that have contributed to the project.

Current repositories would likely be moved to the organization would be:

• This repository, feather-mappings
• The intermediary repository
• The 1.7.2 example mod repository
• @viktor40's FeatherScripts repository
• Maybe Vik's fork of Enigma, but ideally this would be an "unforked" repository

After repositories are moved, we have to make sure any forks referencing this repository don't somehow break, and that git remote references to this repository are changed accordingly, and to remember to change the link pointing to the Intermediary repository.

As we are not using the so-called "cleanroom policy" that Yarn is, the contributing guidelines should be updated to reflect that.

Type
Method

Describe where the element is
Lnet/minecraft/client/render/entity/EntityRenderer;renderAreaEffectCloud(Lnet/minecraft/util/math/Box;DDD)V

Description of the element
It is used in DefaultRenderer with the first parameter set to entity.shape, this results in a white cube of the size of the hitbox being rendered. It is not used anywhere else.

Expected name
renderWhiteBox

Additional Context
beta 1.7.3
image of DefaultRenderer, whose only purpose is to invoke "renderAreaEffectCloud"

can look at both Yarn and QM for how this needs to be implemented