GlowstoneMC / Glowstone-Legacy

An open-source server for the Bukkit Minecraft modding interface
Other
363 stars 122 forks source link

Add gamerules implementation #583

Closed jimmikaelkael closed 9 years ago

jimmikaelkael commented 9 years ago

This PR adds default vanilla game rules for the /gamerule command.

I've linked the "doDaylightCycle" option to the world time.

Note: currently, GlowKit gamerule command implementation is blocking the user to add custom gamerules. I found it weird and I'd like to have your point of view: do we really want to restrict it ?

Related issue/PR:

Aaron1011 commented 9 years ago

In vanilla 1.8, it's possible to have custom gamerules, so I think there's no reason to restrict it.

jimmikaelkael commented 9 years ago

It's as easy as to remove the isGameRule() method call in their implementation ;)

turt2live commented 9 years ago

The enum is inappropriate due to the nature of game rules. A string-based system is ideal.

jimmikaelkael commented 9 years ago

String ? Seriously I think using a gameRules.getBooleanValue("doDaylightCycle") is not so fun. We're not going to break anything with this enum if they are changing names since it's only on Glowstone implementation side.

EDIT: If you look at the GameRuleManager, it's in fact already internally string-based (like you said this is due to the way gamerules are stored), there are only some typed getters to access the desired gamerule value. The enum is there to allow an easier access to the gamerule options for the developers so that they don't have to remember the string.

turt2live commented 9 years ago

The enum is an extra layer of unneeded complexity. Instead, I would shorten the method names so they are not so verbose. If the enum actually did something, then I would be more inclined to have it.

jimmikaelkael commented 9 years ago

I agree it's not really needed, I just tought that an enum could be less error prone than strings. I'll apply requested changes asap.

jimmikaelkael commented 9 years ago

@turt2live I applied the requested changes.

turt2live commented 9 years ago

@jimmikaelkael when you get the chance, please review the pull request I made to your branch :3

jimmikaelkael commented 9 years ago

@turt2live Now I see what you meant by string-based system. This is merged, thank you ;)

SpaceManiac commented 9 years ago

Merged in 2634e8d2a4291cad47854e871033043d79c1f489.