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

Entity Object Builders #67

Merged
merged 40 commits into from Oct 8, 2022
Merged

Entity Object Builders #67

merged 40 commits into from Oct 8, 2022

Conversation

Platymemo
Copy link
Contributor

@Platymemo Platymemo commented Jan 18, 2022 •
edited

This PR adds:

  • A new library, entity identical to the one in Entity Events module #42
  • Four new modules, entity, points_of_interest, vehicle and villager.
    • Each module provides helpers and builders to register their relevant values.

OroArmor
OroArmor approved these changes Jan 18, 2022
View reviewed changes
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.

Descriptions could be improved slightly.

@OroArmor
Copy link
Member

I believe that entity_types and entity_attributes are general enough to entities to be merged into a entity module. Similar to how the item library has the item module.

@OroArmor
Copy link
Member

The real question is where something like the EntityRendererRegistry stuff should go, since that's both client and entity. Personally I think entity/entity (see above) is fine for it, as that's where it will most likely be used from, but it could be in other modules.

@LambdAurora LambdAurora added enhancement New feature or request library: entity Related to the entity library. new: library A pull request which adds a new library. new: module A pull request which adds a new module test labels Jan 18, 2022
@LambdAurora LambdAurora added this to the Initial release milestone Jan 18, 2022
LambdAurora
LambdAurora reviewed Jan 18, 2022
View reviewed changes
Copy link
Member

@LambdAurora LambdAurora left a comment

Choose a reason for hiding this comment

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

Also general javadoc note: @param, @return, etc annotations should not end with punctuation.

...ary/entity/entity_types/src/main/java/org/quiltmc/qsl/entity_types/impl/QuiltEntityType.java Outdated Show resolved Hide resolved
..._of_interest/src/main/java/org/quiltmc/qsl/points_of_interest/api/PointOfInterestHelper.java Show resolved Hide resolved
..._of_interest/src/main/java/org/quiltmc/qsl/points_of_interest/api/PointOfInterestHelper.java Outdated Show resolved Hide resolved
@Platymemo
Copy link
Contributor Author

I believe that entity_types and entity_attributes are general enough to entities to be merged into a entity module. Similar to how the item library has the item module.

I went ahead and did that, I think it makes sense and it makes the javadocs play nicely

@Platymemo
Copy link
Contributor Author

Sorry about the force-pushes, I rebased instead of merging and did a dumb and I won't be doing that again.

@TheGlitch76 TheGlitch76 self-requested a review August 28, 2022 16:09
TheGlitch76
TheGlitch76 reviewed Aug 28, 2022
View reviewed changes
Copy link
Member

@TheGlitch76 TheGlitch76 left a comment

Choose a reason for hiding this comment

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

oops, accidentally requested myself for review

@TheGlitch76 TheGlitch76 added library: entity Related to the entity library. and removed test library: entity Related to the entity library. new: library A pull request which adds a new library. labels Aug 28, 2022
@EnnuiL EnnuiL added s: tested This pull request has been tested and confirmed as working. t: new api This adds a new API. final-comment-period and removed s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. enhancement New feature or request labels Sep 29, 2022
Leo40Git
Leo40Git suggested changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

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

Some small nitpicks regarding the Points of Interest stuff.

I'm a bit concerned about the Trade Offer API as well, it seems to be a port of Fabric API's stuff - which I recall being very broken. Correct me if I'm wrong, of course!

...est/src/main/java/org/quiltmc/qsl/points_of_interest/impl/PointOfInterestTypeExtensions.java Outdated Show resolved Hide resolved
...nterest/src/main/java/org/quiltmc/qsl/points_of_interest/mixin/PointOfInterestTypeMixin.java Outdated Show resolved Hide resolved
EnnuiL
EnnuiL suggested changes Oct 5, 2022
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.

Sorry for this last-minute review, but it was necessary; If you could double-check with Checkstyle (although remembering to not trust it for

paragraphs), I'd appreciate it; oh, also, we still have gaps on API's javadoc, those should be covered; don't only apply my suggestions :P

library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
library/entity/entity/src/main/java/org/quiltmc/qsl/entity/api/QuiltEntityTypeBuilder.java Outdated Show resolved Hide resolved
..._of_interest/src/main/java/org/quiltmc/qsl/points_of_interest/api/PointOfInterestHelper.java Outdated Show resolved Hide resolved
library/entity/vehicle/src/main/java/org/quiltmc/qsl/vehicle/api/MinecartComparatorLogic.java Outdated Show resolved Hide resolved
library/entity/villager/src/main/java/org/quiltmc/qsl/villager/api/TradeOfferHelper.java Outdated Show resolved Hide resolved
Platymemo and others added 3 commits October 5, 2022 14:57
@Platymemo @EnnuiL @Leo40Git
Apply suggestions
310e485 
Co-authored-by: Ennui Langeweile <85590273+EnnuiL@users.noreply.github.com>
Co-authored-by: ADudeCalledLeo <7997354+Leo40Git@users.noreply.github.com>
@Platymemo
checkstyle, docs, and name changes
d3feb32
@Platymemo
un-inject internal interface
a67850d
@Platymemo Platymemo requested review from EnnuiL and Leo40Git and removed request for EnnuiL October 6, 2022 23:10
Leo40Git
Leo40Git approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@Leo40Git Leo40Git left a comment

Choose a reason for hiding this comment

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

LGTM!

EnnuiL
EnnuiL approved these changes Oct 8, 2022
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.

This seems good to merge now

@EnnuiL EnnuiL merged commit 147cfcb into QuiltMC:1.19 Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LambdAurora LambdAurora LambdAurora left review comments

@TheGlitch76 TheGlitch76 TheGlitch76 left review comments

@williambl williambl williambl approved these changes

@Leo40Git Leo40Git Leo40Git approved these changes

@OroArmor OroArmor OroArmor approved these changes

@EnnuiL EnnuiL EnnuiL approved these changes

Assignees
No one assigned
Labels
final-comment-period library: entity Related to the entity library. new: module A pull request which adds a new module s: tested This pull request has been tested and confirmed as working. t: new api This adds a new API.
Projects
None yet
Milestone
Initial release
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Platymemo @OroArmor @oliviathevampire @williambl @Leo40Git @LambdAurora @TheGlitch76 @EnnuiL