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.

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

Fix Gradle and add some new Item mappings #3

Open
wants to merge 28 commits into
base: b1.7.3
Choose a base branch
from

Conversation

Asbestosstar
Copy link

Hola, the gradle requested a nonexistant folder in the nonexistant master branch of the old intermediaries repo, i have fixed this replacing it with the better main branch and the actual folder name and also brought it down 1.7.3 and added many new item mappings

@Asbestosstar
Copy link
Author

added a few more

not perfect, especially on field names will need some updating
Copy link
Collaborator

@sschr15 sschr15 left a comment

Choose a reason for hiding this comment

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

Plentiful additional mappings, but lots of inconsistencies with both our own styling and other code also contributed in this PR

mappings/net/minecraft/block/Block.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/block/Block.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/class_264.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/Item.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/Item.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/Item.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/map/MapState.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/map/MapState.mapping Outdated Show resolved Hide resolved
@Asbestosstar
Copy link
Author

OK i think i made the changes

Copy link

@lenrik1589 lenrik1589 left a comment

Choose a reason for hiding this comment

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

i read, a little

mappings/net/minecraft/item/Item.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/Item.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/ItemStack.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/ItemStack.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/ItemStack.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/item/SoupItem.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/item/SugarcaneItem.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/item/SwordItem.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/item/ToolMaterial.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/item/map/MapState.mapping Outdated Show resolved Hide resolved
Copy link
Member

@Ampflower Ampflower left a comment

Choose a reason for hiding this comment

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

A couple naming changes that could be implemented; otherwise seems fine to me.

@@ -0,0 +1,14 @@
CLASS net/minecraft/class_865 net/minecraft/item/map/MapState
Copy link
Member

Choose a reason for hiding this comment

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

Seems like an odd name for this. Perhaps MapData?

mappings/net/minecraft/block/Block.mapping Outdated Show resolved Hide resolved
Copy link
Member

@Ampflower Ampflower left a comment

Choose a reason for hiding this comment

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

Some changes to be requested purely from naming and conflicts asides

FIELD field_1347 air I
FIELD field_1348 touchingWater Z
FIELD field_1512 firstUpdate Z
FIELD field_1691 skinURL Ljava/lang/String;
Copy link
Member

Choose a reason for hiding this comment

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

Would prefer Url but is just kinda personal convention; and I don't think we ever decided on a convention for here.

FIELD field_2746 renderDistanceMultiplier D
FIELD field_2903 isFireImmune Z
FIELD field_3004 attacked Z
FIELD field_3103 capeURL Ljava/lang/String;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Comment on lines +4 to +5
FIELD field_2704 iDToEntity Ljava/util/Map;
FIELD field_2705 entityToID Ljava/util/Map;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FIELD field_2704 iDToEntity Ljava/util/Map;
FIELD field_2705 entityToID Ljava/util/Map;
FIELD field_2704 IdToEntity Ljava/util/Map;
FIELD field_2705 entityToId Ljava/util/Map;

Comment on lines +9 to +10
METHOD method_1401 create (Ljava/lang/String;Lnet/minecraft/class_14;)Lnet/minecraft/class_1;
METHOD method_1927 create (ILnet/minecraft/class_14;)Lnet/minecraft/class_1;
Copy link
Member

Choose a reason for hiding this comment

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

Probably could be helpful to have a clear semantic in method name but it's hard to tell without context

Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn't exist as mappings/net/minecraft/item/map/MapCoordinates.mapping takes its place.

Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn't exist as mappings/net/minecraft/item/map/MapState.mapping takes its place.

Actually, it is unclear which one should be the primary one.

Copy link
Member

Choose a reason for hiding this comment

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

Conflicted with mappings/net/minecraft/item/MapState.mapping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants