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

Added gamerule command #194

Closed
wants to merge 18 commits into from
Closed

Added gamerule command #194

wants to merge 18 commits into from

Conversation

williamhatcher
Copy link
Contributor

/gamerule will list all game rules, show the value of a game rule, or set a game rule.

This does not implement the actual game rules!

Also, there are a few differences between this and the vanilla command.

  • Trying to set spawnRadius to hello or anything other than a number will fail (try it out in vanilla!)
  • Tab-completion does not work. I would have to specify the default game rules as subcommands in the Info.lua file, and that would prevent the inclusion of custom game rules.
  • There are a few other minor differences, but I've done my best to make it compatible with the vanilla command.

There's a bug that I've found, can reproduce, but can't find a pattern to.
When you try to retrieve a previously set game rule, the plugin cannot recall it in the GameRules table, but the rule is visible when you list the game rules. (also, the value is stored)

@williamhatcher
Copy link
Contributor Author

Can I get some feedback on this?

@mathiascode
Copy link
Member

Gamerules are per world, so I think it would make more sense to store them in the world.ini file. The best would be to store them in level.dat, since vanilla does that, but that would probably require some changes in Cuberite itself.

@williamhatcher
Copy link
Contributor Author

I noticed that the default gamerules.ini file wasn't pushed, and fixed it.

I can move the game rules to the world.ini file. This will add a small amount of overhead, but I think it will be worth it.

@madmaxoft
Copy link
Member

One thing I'm definitely against is having a hardcoded path for the INI file (like you did in commit 2f14d0d). The original version was better; storing them per-world would be the best, of course. Somehow CircleCI won't let me see the error I assume you were fixing with the hardcoded path, it's most likely a bug in the plugin checker.

Copy link
Member

@madmaxoft madmaxoft left a comment

Choose a reason for hiding this comment

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

Could we simplify by pretending all gamerules are string values and only their interpreter (once implemented) will convert such a value to whatever type it needs? It would simplify a lot of stuff.

Alternatively, use the string handling for all unknown gamerules, and for the known ones, we'll have a table of them and their handlers somewhere, so that table could also specify the type of the parameter.

Info.lua Outdated
},
{
Params = "rule value",
Help = "Sets a game rule value",
Copy link
Member

Choose a reason for hiding this comment

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

Probably should get a "WorldName" param, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I move the rules to the world ini, I'll change this

Copy link
Member

Choose a reason for hiding this comment

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

The params should be correct from the start, even if not used. Makes migration easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this

Info.lua Outdated
Description = "Allows players to set and query game rule values",
RecommendedGroups = "admins",
},

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to have a separate permission for querying, and a separate one for setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I'll add them to the permissions table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madmaxoft Should I have core.gamerule allow setting and querying,
core.gamerule.query for querying and listing only, and
core.gamerule.set for setting only or setting and querying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, I just made the description vague

gamerule.lua Outdated
local grDB
local ini = cIniFile()
local boolean = "boolean"
local number = "number"
Copy link
Member

Choose a reason for hiding this comment

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

These two lines are rather weird, can you change the name of the variables to something more descriptive of what they're used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They won't be needed when I remove the game rule types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, removed.

@@ -0,0 +1,75 @@
;Game Rules
;This file contains all the set game rules
;All values here can be configured using the webadmin interface, if enabled
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any webadmin handlers to support this claim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't added this yet, but rest assured that it will be added.

@madmaxoft
Copy link
Member

It'd be great to have at least one gamerule actually implemented, so that we can see how it integrates with this groundwork.

@williamhatcher
Copy link
Contributor Author

williamhatcher commented Aug 23, 2017

Could we simplify by pretending all gamerules are string values and only their interpreter (once implemented) will convert such a value to whatever type it needs? It would simplify a lot of stuff.

The problem with this is that there's no validation when setting game rules, also, some of the interpreters will be called lots of times, mobGriefing for example, and It would be very slow if they had to convert the value each time they were called. Maybe interpreters could register a function for validation?
Maybe like: AddValidation(a_Rule, a_Callback) where a_Rule is a string, and a_Callback is a function?

If cPluginManager:RemoveHook existed, then the plugin could add and remove hook callbacks as needed, but right now, all possible hook callbacks have to be registered, and ran each time a hook emits. This forces the interpreter to check if it should even do anything each time it is ran.

@williamhatcher
Copy link
Contributor Author

williamhatcher commented Aug 24, 2017

Wow!

I changed a bunch of stuff. Here's a summary:

  • Added a new validation system like WE's hook system.
  • Switched to world based game rules
  • Added world parameters (but they aren't implemented)
  • Tried to clean up the code
  • Separated the large file into two smaller files
  • I'm really really sorry for the super large commit.

@williamhatcher
Copy link
Contributor Author

@madmaxoft, What do you think of the changes so far?

Removed duplicate code and made setting values more flexable.
@williamhatcher
Copy link
Contributor Author

Now the console and in game commands work the same way.
You are now able to set values with spaces in them!

@williamhatcher
Copy link
Contributor Author

Note to self: have SaveValue store the value in GameRules table

@williamhatcher
Copy link
Contributor Author

@madmaxoft do you mind looking at my new commits?

@mathiascode
Copy link
Member

This should probably be implemented as a proper API in the server core.

@mathiascode mathiascode closed this Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants