-
Notifications
You must be signed in to change notification settings - Fork 56
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
Implement limitsets #454
Open
marchc1
wants to merge
22
commits into
dev
Choose a base branch
from
limitsets
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Implement limitsets #454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
will change things around later
This seems to work perfectly fine. This solves an issue where ACF persist data doesn't get reloaded when acf_reload is called.
Note: Probably should be checked against hook docs standards before dev -> master merge This is needed because we want to hook when the user sets the server data key even if it hasn't changed (allows the limitset notice to disappear if they pick "none" for example)
Execute() only calls stuff on the server, but the data (Name, Author, Description, etc) could be shared; which would simplify the limitset menu
Eventually, we should merge something into the menu for this, but this will work for now
Another nice thing about this I forgot to mention; The API for creating limitsets is publicly exposed, so if someone wanted to create their own custom limitset and distribute it, they could easily do so by placing a file in lua/acf/limitsets/limitsets/ like this... local YourLimitsetHere = ACF.LimitSets.Create("I Hate Links")
YourLimitsetHere:WithAuthor("Not the ACF Team")
YourLimitsetHere:WithDescription("Grrr")
YourLimitsetHere:SetServerData("LinkDistance", 0) |
We'd need to add some kind of SetGlobal method or something, which could end up being tricky... We could also just set up LinkDistance & MobilityLinkDistance as server-data now before PR'ing; up to others how we approach this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Limitsets are collections of server-side settings, which when selected can automatically update as we fine tune combat in the future. We can also leave Sandbox alone for the most part (for example, mobility link changes could be placed in Combat instead while Sandbox can leave them as is).
The default value of ACF.SelectedLimitset is set to "none", which is a reserved name for no limitset and will not modify any settings. This should keep server owners happy and not modify their settings without their permission.
To ensure that limitsets have the opportunity to be used and don't just rot on 95% of servers, server owners/superadmins will be given a popup when they receive this update. The popup will tell them about the difference between the two modes, and offer a "Custom" mode (which in reality uses no limitset and sets ACF.SelectedLimitset to "none").
Once they make a choice on which limitset to use (or not to use the feature), an internal convar is set to 1, which will ensure the message does not pop up again (unless they're doing something funky). There is probably a better way to do this, but this is the best way I can think of right now.
For the Future
In the future, we should consider if we want to keep this menu popup but re-write some of the text (ie. remove the "This is a new feature" stuff), or remove the popup altogether. I am in favor of leaving the popup if convars are reliable enough for this and no issues occur.
Also, for future settings we may implement, I think this is how we should do it:
and with existing settings:
Tldr; we should leave Sandbox alone where possible, and provide a much better moderation experience for server staff and a much better combat experience for everyone with Combat. If a server doesn't want to use limitsets, the default values from globals.lua should represent what Combat mode sets, and its up to server staff to adjust them if they want to. This should satisfy all parties that use ACF:
Other Notes
(it should be obvious, but for the layman reading this PR; we don't think restrictions are the end-all-be-all to improving combat, obviously additions need to happen too and various other balancing fixes; this would just allow us to do much more of that while still keeping builders a lot happier than us hard enforcing things within the addon)