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

Game Rule API #55

Closed
wants to merge 3 commits into from
Closed

Game Rule API #55

wants to merge 3 commits into from

Conversation

IotaBread
Copy link
Member

Port of Fabric's Game Rule API

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.

Uuh, from my own experiences on making Boring Default Game Rules and Damage Incorporated, Fabric's Game Rule API needs some improvements
The order of the game rule categories are scrambled even if they are all registered by one mod, the name text of enum rules has an inadequate width for wrapping, and compared to Vanilla's game rule types, the Fabric ones are unnecessarily restrictive on the access of its internals (in fact, I ended needing to create Accessors, and all of them were on Fabric API's rules)
I think a revamp of the module similar to the Quilt Tags API one would be more appropriate instead of a direct port, but here's my suggestions of what could be done now:

private static final Logger LOGGER = LogManager.getLogger(GameRuleRegistry.class);

private final int minimumValue;
private final int maximumValue;
Copy link
Contributor

@EnnuiL EnnuiL Nov 27, 2021

Choose a reason for hiding this comment

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

Both minimum and maximum values need getters; The reason for that is that it may be useful for a mod that needs to directly access a game rule's bound (example: Boring Default Game Rules and its generation of a JSON Schema based on the game rules)

private static final Logger LOGGER = LogManager.getLogger(GameRuleRegistry.class);

private final double minimumValue;
private final double maximumValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

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.

oh whoops, forgot this one

private static final Logger LOGGER = LogManager.getLogger(GameRuleRegistry.class);

private final Class<E> classType;
private final List<E> supportedValues;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think supportedValues also needs a getter too

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.

Oh, and finally, DoubleRule needs a void set(double, MinecraftServer) method; The vanilla ones have it, why not DoubleRule?

@LambdAurora LambdAurora added enhancement New feature or request library: data Related to the data library. new: module A pull request which adds a new module test new: library A pull request which adds a new library. and removed library: data Related to the data library. labels Nov 29, 2021
@i509VCB
Copy link
Contributor

i509VCB commented Dec 3, 2021

As the primary author of the game rules api from fabric API, I grant QuiltMC license to the code under the Apache 2.0 license.

For the sake of not causing more license issues, I wouldn't worry about the headers in the impl and mixin packages or the testmod source set.

@TheGlitch76 TheGlitch76 mentioned this pull request Dec 4, 2021
2 tasks
@LambdAurora LambdAurora added this to the Initial release milestone Dec 6, 2021
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.

The lack of a void set(double, MinecraftServer) method for DoubleRule still needs to be addressed, otherwise, it's fine for now

@TheGlitch76
Copy link
Member

My only concern here is that now that i'm actually staring at it a lot, content_other is a really awkward name for a library. I think it should have been other_content in the first place.

cc @LambdAurora

/**
* Utility class for creating custom game rule categories outside of the categories {@link GameRules.Category Minecraft provides}.
*/
public final class CustomGameRuleCategory {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be a record?

@LambdAurora LambdAurora added the s: outdated This pull request is outdated. label May 20, 2022
@TheGlitch76
Copy link
Member

Hi--this PR seems to be unmaintained right now. I'm converting it to a draft to signify that it's not in a mergable state.

Feel free to mark it as ready for review once you begin working on this again.

@TheGlitch76 TheGlitch76 marked this pull request as draft August 22, 2022 22:18
@EnnuiL
Copy link
Contributor

EnnuiL commented Aug 23, 2022

About a Game Rules API, I believe that a direct port of Fabric's one will not be adequate here. With that (and also the inactivity), I'm closing this PR

@EnnuiL EnnuiL closed this Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request new: library A pull request which adds a new library. new: module A pull request which adds a new module s: outdated This pull request is outdated.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants