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.

Sign up for GitHub

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

Jump to bottom

Advancements #496

Merged
merged 10 commits into from
Nov 10, 2023
Merged

Advancements #496

merged 10 commits into from
Nov 10, 2023

Conversation

Platymemo
Copy link
Contributor

This is a huge advancement refactor/update.

The most major change is:
Criterion -> CriterionTrigger which was changed due to string references in codecs calling it "trigger"

Additionally, some small methods/fields were renamed for consistency/better naming

With this PR, it should be at 100% completion.

@OroArmor
Copy link
Member

before i review: aH +700

@ix0rai ix0rai added t: refactor proposes a refactor t: new adds new mappings v: snapshot targets a snapshot version of minecraft s: large PRs with more than 700 lines labels Sep 16, 2023
@ix0rai ix0rai self-requested a review September 16, 2023 15:56
supersaiyansubtlety
supersaiyansubtlety requested changes Sep 17, 2023
View reviewed changes
Copy link
Contributor

@supersaiyansubtlety supersaiyansubtlety left a comment

Choose a reason for hiding this comment

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

Not a complete review, but figured I'd submit what I have so far

mappings/net/minecraft/advancement/AdvancementProgress.mapping Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementProgress.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementProgress.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementProgress.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementProgress.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementRequirements.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementTree.mapping Show resolved Hide resolved
mappings/net/minecraft/advancement/criterion/CriterionTrigger.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/AdvancementCriterion.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/advancement/criterion/CuredZombieVillagerCriterion.mapping Show resolved Hide resolved
EnnuiL
EnnuiL suggested changes Sep 17, 2023
View reviewed changes
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.

question: shouldn't Criterions be renamed too? i have prototyped a registry for 1.20.1, so i am curious about that

supersaiyansubtlety
supersaiyansubtlety requested changes Sep 17, 2023
View reviewed changes
mappings/net/minecraft/network/packet/s2c/play/AdvancementUpdateS2CPacket.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/ContextAwarePredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Outdated Show resolved Hide resolved
ix0rai
ix0rai reviewed Sep 18, 2023
View reviewed changes
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.

this is fantastic coverage, address @supersaiyansubtlety's suggestions and I'll approve!

mappings/net/minecraft/advancement/AdvancementProgress.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/predicate/ContextAwarePredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/ContextAwarePredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Show resolved Hide resolved
mappings/net/minecraft/predicate/entity/EntityPredicate.mapping Show resolved Hide resolved
@Platymemo
Copy link
Contributor Author

question: shouldn't Criterions be renamed too? i have prototyped a registry for 1.20.1, so i am curious about that

I was originally thinking of it, but my fingers were tired. I can totally rename them though

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

🚀 Target branch has been updated to 1.20.2-rc2

@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 github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Sep 20, 2023
@Antikyth Antikyth added the update-base used to notify github actions that the base branch should be updated label Sep 23, 2023
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 1.20.2

@github-actions github-actions bot changed the base branch from 1.20.2-rc2 to 1.20.2 September 23, 2023 01:10
@github-actions
Copy link
Contributor

🚨 Please fix merge conflicts before this can be merged

@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
@github-actions
Copy link
Contributor

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Oct 16, 2023
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Oct 20, 2023
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 23w42a

@github-actions github-actions bot changed the base branch from 23w41a to 23w42a October 20, 2023 04:27
@github-actions
Copy link
Contributor

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Oct 20, 2023
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Oct 27, 2023
@github-actions github-actions bot changed the base branch from 23w42a to 23w43a October 27, 2023 00:53
@github-actions
Copy link
Contributor

🚀 Target branch has been updated to 23w43a

@github-actions
Copy link
Contributor

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Oct 27, 2023
@ix0rai ix0rai added the update-base used to notify github actions that the base branch should be updated label Nov 2, 2023
@github-actions github-actions bot changed the base branch from 23w43a to 23w44a November 2, 2023 16:53
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 2, 2023

🚀 Target branch has been updated to 23w44a

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 2, 2023

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot removed the update-base used to notify github actions that the base branch should be updated label Nov 2, 2023
@ix0rai
Merge branch '23w44a' into advancements
42f2ae6 
# Conflicts:
#	mappings/net/minecraft/recipe/RecipeUnlocker.mapping
#	mappings/net/minecraft/util/dynamic/Codecs.mapping
@ix0rai
resolve comments
9735c76
@EnnuiL EnnuiL removed their request for review November 4, 2023 19:02
@ix0rai ix0rai removed the outdated this pull request hasn't been updated to the latest version or has merge conflicts label Nov 6, 2023
@ix0rai ix0rai mentioned this pull request Nov 6, 2023
Merged
@supersaiyansubtlety supersaiyansubtlety added the final-comment-period is approved and will soon be merged if no issues are raised label Nov 6, 2023
@ix0rai ix0rai merged commit f841c90 into QuiltMC:23w44a Nov 10, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ix0rai ix0rai ix0rai left review comments

@EnnuiL EnnuiL EnnuiL approved these changes

@OroArmor OroArmor OroArmor approved these changes

@supersaiyansubtlety supersaiyansubtlety supersaiyansubtlety approved these changes

Assignees
No one assigned
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: snapshot targets a snapshot version of minecraft
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Platymemo @OroArmor @EnnuiL @ix0rai @supersaiyansubtlety @Antikyth