Change up game rules to be more flexible and follow Sponge practices by parlough · Pull Request #1745 · SpongePowered/SpongeAPI

parlough · 2018-01-23T01:51:18Z

Follows the style the rest of Sponge uses better while also providing a more flexible system.

Looking for some input on the set-up I have so far, then I will flesh out the rest of the API and write the implementation.

@Cybermaxke Any input here?

parlough · 2018-01-23T02:12:24Z

Locally the file GameRule.java has the right name but Git seems to think the name is Gamerule.java :/

ryantheleach · 2018-01-23T02:31:11Z

src/main/java/org/spongepowered/api/world/gamerule/GameRuleHolder.java

+import java.util.Optional;
+
+/**
+ * Someone who holds various {@link GameRule}s and the values they point to.


Someone? Were you going to delegate per user gamerules to the world properties that currently controls them?

I meant "Something", just in-case an implementation wants to allow gamerules to extend beyond just a WorldProperties.

ryantheleach · 2018-01-23T02:35:55Z

If you are going to refactor GameRules I'm expecting that you are going to come up with some sort of sharing context?

e.g. nether and the_end and modded dimenson-worlds by default on sponge delegate to the overworld gamerules?

Where as Sponge worlds can share a 'multiverse'/world that contains the gamerules for a group of dimensions?

Note: My use of multiverse is just to convey concept, I am utterly aware how much of a Pain in the ass it would be to globally have support for true multiverses separating out inventory, stats, recipes. plugins etc on a Mojang based server.

I just can't help but think that the concept would be valuable, even if it only see's limited use on SpongeForge / SpongeVanilla. There isn't anything really forcing Lantern to having such limitations.

parlough · 2018-01-23T02:51:37Z

src/main/java/org/spongepowered/api/world/gamerule/GameRuleType.java

+ * @param <T> The value type of the game rule
+ */
+@CatalogedBy(GameRuleTypes.class)
+public interface GameRuleType<T> extends CatalogType {


I think I'll introduce a builder for this to make it easier to create a new one.

parlough · 2018-01-23T02:51:59Z

src/main/java/org/spongepowered/api/world/gamerule/GameRule.java

+ * @param <T> The value type of the game rule
+ */
+@CatalogedBy(GameRules.class)
+public interface GameRule<T> extends CatalogType {


Will create a builder for this as well.

gabizou · 2018-01-23T07:35:28Z

src/main/java/org/spongepowered/api/world/gamerule/GameRuleType.java

+     *
+     * @return This type's deserializer
+     */
+    Function<String, T> getDeserializer();


It’s not really a serializer, it’s more of a printer/parser combination, isn’t it?

Cybermaxke · 2018-01-23T12:33:57Z

src/main/java/org/spongepowered/api/world/World.java

@@ -334,28 +332,6 @@ default Difficulty getDifficulty() {
        return getProperties().getDifficulty();
    }



Can you make the world also a GameRuleHolder? And then delegate all the methods to the properties.

Yeah, I will just specify in the Javadoc that most implementations delegate the methods to the properties.

Cybermaxke · 2018-01-23T12:38:26Z

src/main/java/org/spongepowered/api/world/gamerule/GameRuleHolder.java

+     * @param value The value to set the game rule to
+     * @param <T> The value type of the game rule
+     */
+    <T> void setGameRule(GameRule<T> gameRule, T value);


This method should also call the ChangeWorldGameRuleEvent, so return a boolean whether it was successful? That said, the ChangeWorldGameRuleEvent should also be refactored for the new API.

Ah yes I forgot to do this, thanks for reminding me. Will make these changes.

dualspiral · 2018-01-23T16:35:30Z

While I know we're breaking commands and text for API 8 without deprecations, here, I'd argue deprecations serve a purpose. Deprecations will emit a warning on compilers that use the code to point them in the right direction - while we'll really hammer the change in commands when API 8 is out, game rules and smaller, easier to deprecate systems might end up falling by the wayside. We should make sure we are in the mindset of deprecating when we can, not just using major API versions as a reason to break with no consequence.

That being said, looking at this, I suspect it's possible for both systems to coexist peacefully with no breaking changes. Do you think you could do that in time for 7.1? If so, maybe that's a route forward - a deprecation in 7.1 with the new methods, removal in 8.

parlough · 2018-01-23T16:41:00Z

@dualspiral I believe this is doable to an extent, the only problem would be for the events. I could introduce an event with a different name or package that supports the new changes while deprecating the old one.

How would you feel about that solution?

gabizou · 2018-08-30T05:45:35Z

@dualspiral I disagree with trying to maintain deprecation policy for the bleeding branch. As we're already doing massive refactors left and right (Text has already changed, Value API is about to change, DataRegistration is partially changing due to values changing, Extent API is 100% rewritten at this point, Scheduling tasks will change as soon as we expose the client in the process, CauseStackManager is getting a rename, some major events are being redone, Inventory API changed, commands are going to be changing), I don't feel it necessary to keep something as simple as game rules deprecated for a version and then break it in a little way afterwards.

Not to mention that documentation alone for the migration from API 7 to API 8 will be somewhat extensive at that point given the PR's that were used to make all these changes, it'll be easy to show the migration via test plugin changes needed between the various pr's.

dualspiral · 2018-08-30T08:40:08Z

@gabizou With the extent of changes since, where before, it was impossible to know with clarity how much we would need to change, sure. Deprecations do serve a purpose and we should still pay mind to their use, but given how much we've changed since this was opened, I see the point.

Do bear in mind I made that comment 7 months ago, just after 7 was released. Things have changed!

gabizou · 2018-08-30T08:43:51Z

Do bear in mind I made that comment 7 months ago, just after 7 was released. Things have changed!

Of course, just resuscitating PR's (having went through I think all the open PR's currently) and making sure their either alive, or relegated for future development/management.

Cybermaxke · 2019-05-01T22:22:21Z

The game rule is already refactored in the api-8 branch, comment on the status PR #1995 if there's anything missing.

parlough added status: input wanted implementation: pending system: world labels Jan 23, 2018

parlough added this to the Revision 8.0 milestone Jan 23, 2018

parlough requested a review from kashike as a code owner January 23, 2018 01:51

parlough force-pushed the feature/game-rules branch 2 times, most recently from 7321518 to c497773 Compare January 23, 2018 02:03

Change up game rules to be more flexible and follow Sponge practices

a4dc697

parlough force-pushed the feature/game-rules branch from c497773 to a4dc697 Compare January 23, 2018 02:14

ryantheleach reviewed Jan 23, 2018

View reviewed changes

parlough added the status: wip label Jan 23, 2018

parlough commented Jan 23, 2018

View reviewed changes

gabizou reviewed Jan 23, 2018

View reviewed changes

Cybermaxke reviewed Jan 23, 2018

View reviewed changes

gabizou added branch: bleeding api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) labels Aug 30, 2018

Cybermaxke closed this May 1, 2019

		@@ -334,28 +332,6 @@ default Difficulty getDifficulty() {
		return getProperties().getDifficulty();
		}

Uh oh!

Conversation

parlough commented Jan 23, 2018

Uh oh!

parlough commented Jan 23, 2018

Uh oh!

ryantheleach Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parlough Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryantheleach commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parlough Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

parlough Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

gabizou Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

Cybermaxke Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parlough Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

Cybermaxke Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

parlough Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

dualspiral commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parlough commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabizou commented Aug 30, 2018

Uh oh!

dualspiral commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabizou commented Aug 30, 2018

Uh oh!

Cybermaxke commented May 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ryantheleach Jan 23, 2018 •

edited

Loading

parlough Jan 23, 2018 •

edited

Loading

ryantheleach commented Jan 23, 2018 •

edited

Loading

Cybermaxke Jan 23, 2018 •

edited

Loading

dualspiral commented Jan 23, 2018 •

edited

Loading

parlough commented Jan 23, 2018 •

edited

Loading

dualspiral commented Aug 30, 2018 •

edited

Loading