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

first looks into 1.20.4 #516

Merged
merged 4 commits into from
Dec 9, 2023
Merged

first looks into 1.20.4 #516

merged 4 commits into from
Dec 9, 2023

Conversation

moehreag
Copy link
Contributor

@moehreag moehreag commented Dec 8, 2023

These are just mostly the unmapped classes I quickly found names for.

  • mostly new classes that are mostly related to new stuff

Things I'm not sure about:

  • BlockCodecs should probably not be in the block package. Where should it go?

(mostly new classes)
@moehreag moehreag added s: medium PRs with less than 700 lines and more than 200 t: new adds new mappings labels Dec 8, 2023
@ix0rai
Copy link
Member

ix0rai commented Dec 8, 2023

codec stuff goes in util

@ix0rai ix0rai added v: release targets a release version of minecraft reviews needed please review this PR labels Dec 8, 2023
@Patbox
Copy link

Patbox commented Dec 8, 2023

BlockCodecs isn't a correct name, as the bound registry uses id of block_type and that class populates it

@moehreag
Copy link
Contributor Author

moehreag commented Dec 8, 2023

BlockCodecs isn't a correct name, as the bound regsistry uses id of block_type and that class populates it

Hmm... looking at the class again maybe BlockTypeRegistry would be a better name?

@OroArmor
Copy link
Member

OroArmor commented Dec 8, 2023

BlockTypes, follows the Blocks naming scheme

@Patbox
Copy link

Patbox commented Dec 8, 2023

It's not a registry either, the closest thing would be Blocks or Items class

@OroArmor
Copy link
Member

OroArmor commented Dec 8, 2023

Also being in the n.m.block is the right package

@moehreag
Copy link
Contributor Author

moehreag commented Dec 8, 2023

Also being in the n.m.block is the right package

alright!

@ix0rai ix0rai self-requested a review December 8, 2023 22:20
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT QMap doesn't use o for objects unless it's for equals(Ljava/lang/Object;)Z, and I'm not sure about any of the single-letter names of these params (except i).
I think it could make sense to use them for primitives (and maybe Objects), but the convention should be made clear if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT QMap doesn't use o for objects unless it's for equals(Ljava/lang/Object;)Z, and I'm not sure about any of the single-letter names of these params (except i). I think it could make sense to use them for primitives (and maybe Objects), but the convention should be made clear if so.

It is entirely possible that using o for object parameters is something I entirely made up and isn't used anywhere and mentioned in any conventions (except for equals methods)

METHOD m_rxpnwroz toMap (Lcom/mojang/serialization/MapLike;)Ljava/util/Map;
ARG 0 mapLike
METHOD m_sfvskaol (Ljava/lang/Object;)Ljava/lang/Object;
ARG 0 object
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent with other o params

ARG 0 object
METHOD mapBuilder mapBuilder ()Lcom/mojang/serialization/RecordBuilder;
METHOD mergeToList mergeToList (Ljava/lang/Object;Ljava/lang/Object;)Lcom/mojang/serialization/DataResult;
ARG 1 o
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
ARG 1 o
ARG 1 o1

ARG 1 o
ARG 2 o2
METHOD mergeToList mergeToList (Ljava/lang/Object;Ljava/util/List;)Lcom/mojang/serialization/DataResult;
ARG 1 object
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent

ARG 1 o
ARG 2 mapLike
METHOD mergeToMap mergeToMap (Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Lcom/mojang/serialization/DataResult;
ARG 1 o
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
ARG 1 o
ARG 1 o1

ARG 2 map
METHOD remove remove (Ljava/lang/Object;Ljava/lang/String;)Ljava/lang/Object;
ARG 1 map
ARG 2 s
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
ARG 2 s
ARG 2 string

s is used for short in other places, and I think if single letters are used, they should be for primitives (and maybe Objects)

ARG 1 string
CLASS C_wogpegfy MapBuilder
METHOD append (Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
ARG 1 o
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
ARG 1 o
ARG 1 o1

@moehreag moehreag merged commit 8bd599b into QuiltMC:1.20.4 Dec 9, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviews needed please review this PR s: medium PRs with less than 700 lines and more than 200 t: new adds new mappings v: release targets a release version of minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants