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

Big GUI Mappings PR (sorry!) #495

Merged
merged 19 commits into from
Sep 30, 2023
Merged

Conversation

Antikyth
Copy link
Contributor

@Antikyth Antikyth commented Sep 15, 2023

Summary: probably not coming unfortunately, I'm very busy at the moment

Big PR, adds a lot of mappings and refactors some existing ones.

@ix0rai ix0rai added t: refactor proposes a refactor t: new adds new mappings v: snapshot targets a snapshot version of minecraft wip this is a work in progress s: large PRs with more than 700 lines labels Sep 15, 2023
@Antikyth
Copy link
Contributor Author

Antikyth commented Sep 15, 2023

I want to know what these invalid mappings are... (in the errors)

@Antikyth Antikyth marked this pull request as ready for review September 15, 2023 21:44
@supersaiyansubtlety
Copy link
Contributor

supersaiyansubtlety commented Sep 15, 2023

Did a bunch of my comments get duped because there were changes while I was reviewing?
Guess not, the duped ones were 'pending' and I canceled the duped review.

Copy link
Member

@OroArmor OroArmor left a comment

Choose a reason for hiding this comment

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

Looks good with the given changes!

@ix0rai ix0rai self-requested a review September 16, 2023 15:57
@Antikyth
Copy link
Contributor Author

I'll try make the changes today, just a lil busy. Also to add a proper summary.

Copy link
Member

@ix0rai ix0rai left a comment

Choose a reason for hiding this comment

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

refactors are great overall, but I'm not sure I like the ResourcePack to Pack renames, they could pretty easily be mistaken for data pack things like that. thank you for the huge PR!

for my approval:

  • address supersaiyansubtlety's comments
  • justify the pack rename

@Antikyth
Copy link
Contributor Author

refactors are great overall, but I'm not sure I like the ResourcePack to Pack renames, they could pretty easily be mistaken for data pack things like that. thank you for the huge PR!

for my approval:

  • address supersaiyansubtlety's comments
  • justify the pack rename

The pack renames was for exactly that reason: they are data pack things as well. At least, some of them are, I'm not certain if there's some of them that aren't.

@Antikyth
Copy link
Contributor Author

refactors are great overall, but I'm not sure I like the ResourcePack to Pack renames, they could pretty easily be mistaken for data pack things like that. thank you for the huge PR!

for my approval:

  • address supersaiyansubtlety's comments
  • justify the pack rename

The pack renames was for exactly that reason: they are data pack things as well. At least, some of them are, I'm not certain if there's some of them that aren't.

As in, data packs use these classes, they don't have their own.

@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Sep 18, 2023
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 1.20.2-rc1

@github-actions github-actions bot changed the base branch from 1.20.2-pre4 to 1.20.2-rc1 September 18, 2023 02:25
@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Sep 18, 2023
@ix0rai
Copy link
Member

ix0rai commented Sep 18, 2023

oh perfect, address changes and you're golden!

@Antikyth
Copy link
Contributor Author

I don't have the time to address the changes at the moment because I have a very busy week. I feel like it might be good to merge this before 1.20.2 releases, however, as it would lump any breaking changes together. If someone had the time to PR to my branch with the changes then I could merge that; otherwise I'll do it when I have time.

@ix0rai
Copy link
Member

ix0rai commented Sep 18, 2023

@Antikyth would you like me to resolve comments myself?

@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Sep 20, 2023
@github-actions github-actions bot changed the base branch from 1.20.2-rc1 to 1.20.2-rc2 September 20, 2023 23:23
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 1.20.2-rc2

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Sep 20, 2023
@Antikyth
Copy link
Contributor Author

Still have to manually apply some suggestions in Enigma.

@Antikyth
Copy link
Contributor Author

I just rebased it onto the recipe holder changes ^

@Antikyth Antikyth added the update-base used to notify github actions that the base branch should be updated label Sep 23, 2023
@github-actions github-actions bot changed the base branch from 1.20.2-rc2 to 1.20.2 September 23, 2023 01:09
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 1.20.2

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Sep 23, 2023
@Antikyth Antikyth added v: release targets a release version of minecraft and removed v: snapshot targets a snapshot version of minecraft labels Sep 23, 2023
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

one word: damn
great job here!

just one nitpick: i feel like something like "the X coordinate" would be better than "the 'x' coordinate" or "the x coordinate"

ARG 2 layoutSettingsConsumer
CLASS C_ekgfxrpf Orientation
METHOD m_fnqdcoug setRotatedY (Lnet/minecraft/unmapped/C_zuxsitrm$C_isrpishr;II)V
COMMENT Sets the "y" coordinate of the given {@code widget} rotated with this orientation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COMMENT Sets the "y" coordinate of the given {@code widget} rotated with this orientation.
COMMENT Sets the Y coordinate of the given {@code widget} rotated with this orientation.

Copy link
Contributor

@TheGlitch76 TheGlitch76 Sep 23, 2023

Choose a reason for hiding this comment

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

No no no! Coordinates are almost always lowercase in math notation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unless you have a very good reason to make it capital, it should be lowercase.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused; i thought this was a standard on our javadocs
And yes, math does have the fancy x and such but we don't have TeX on our javadoc and uh, just having a lowercase letter doesn't make sense here; i made this request because it felt like it blended with the rest of the text, which is bad; it didn't feel distinct, and considering that stuff like Blender does use capital letters for coordinates? I feel like my gut feelings are justified here
Screenshot of Blender showing the transform fields of the default cube, with the usage of capital letters for coordinates being evident
So, if we want to follow a lowercase standard, we ought to use {@code x} in order to make it distinct and we must make it a formal standard; else? It has to be capital

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer flat x over {@code x}, since we usually use code for parameters it may get people to do a double take

Copy link
Contributor

@supersaiyansubtlety supersaiyansubtlety Sep 23, 2023

Choose a reason for hiding this comment

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

  1. I think it should be lowercase because that's how it's used in math
  2. I agree with EnnuiL that "it felt like it blended with the rest of the text, which is bad"
  3. I don't think we should make it distinct with {@code}, because it's not code, that's misleading

So, how about it's made distinct with other text formatting, like some combination of bold/italic/<strong>/etc?
I'm partial to italics, but idk if that's enough to make it stand out in all cases.

Copy link
Member

Choose a reason for hiding this comment

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

I support italics, I don't think it needs to be super flashy

Copy link
Contributor

Choose a reason for hiding this comment

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

Italics is the closest way of making the coordinate letters look like math notation without making Java implement TeX on their javadoc engine :p

Copy link
Contributor Author

@Antikyth Antikyth Sep 25, 2023

Choose a reason for hiding this comment

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

It uses quotes because it's not necessarily the actual y coordinate by the way. I think. On mobile, GitHub won't let me scroll the text to the right to see what it says.

Choose a reason for hiding this comment

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

I definitely wouldnt capitalize it, whatever you pick. The issue isn't so much that coords are generally lowercase in math - it's that uppercase letters have several other connotations! I see an X somewhere and my brain goes "matrix!"

COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual y
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will set its x coordinate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COMMENT it will set its x coordinate.
COMMENT it will set its X coordinate.

Comment on lines +37 to +41
COMMENT Returns the "y" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will be the actual y
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will be its x coordinate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COMMENT Returns the "y" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will be the actual y
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will be its x coordinate.
COMMENT Returns the Y coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will be the actual Y
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will be its X coordinate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again, there is a reason it is in quotes. Perhaps it needs to be made clearer if that wasn't clear, how I don't know.

Comment on lines +65 to +69
COMMENT Returns the "x" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will be the actual x
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will be its y coordinate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COMMENT Returns the "x" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will be the actual x
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will be its y coordinate.
COMMENT Returns the X coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will be the actual X
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will be its Y coordinate.

Comment on lines +72 to +76
COMMENT Sets the "x" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual x
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will set its y coordinate.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
COMMENT Sets the "x" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual x
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will set its y coordinate.
COMMENT Sets the X coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual X
COMMENT coordinate of the {@code widget}. Otherwise, if it is {@link Orientation#VERTICAL VERTICAL},
COMMENT it will set its Y coordinate.

METHOD m_fnqdcoug setRotatedY (Lnet/minecraft/unmapped/C_zuxsitrm$C_isrpishr;II)V
COMMENT Sets the "y" coordinate of the given {@code widget} rotated with this orientation.
COMMENT <p>
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual y
Copy link
Contributor

Choose a reason for hiding this comment

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

oh i missed one

Suggested change
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual y
COMMENT If the orientation is {@link Orientation#HORIZONTAL HORIZONTAL}, this will set the actual Y

@Antikyth
Copy link
Contributor Author

one word: damn
great job here!

just one nitpick: i feel like something like "the X coordinate" would be better than "the 'x' coordinate" or "the x coordinate"

Personally, I don't think so, it's how I'd always write it myself, but if other people agree with that then it can be changed.

@EnnuiL
Copy link
Contributor

EnnuiL commented Sep 30, 2023

ps: do note that as suspected, this was what it was used before
screenshot of X coordinate instances
we shouldn't block this merge though, so,

@EnnuiL EnnuiL merged commit 3e33e4f into QuiltMC:1.20.2 Sep 30, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period is approved and will soon be merged if no issues are raised s: large PRs with more than 700 lines t: new adds new mappings t: refactor proposes a refactor v: release targets a release version of minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants