-
Notifications
You must be signed in to change notification settings - Fork 89
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
Expand visibility controls for objectives #986
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at XML parsing in general the ShowOptions class could probably just be a Set parsed in XMLUtils? I like the simplicity of the implementation though, so if you decide to keep it as-is I have a couple of suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Requested a few minor changes
import tc.oc.pgm.util.xml.InvalidXMLException; | ||
import tc.oc.pgm.util.xml.XMLUtils; | ||
|
||
public class ShowOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should just be a top-level enum
.
public enum ObjectiveOption {
SHOW_MESSAGES("show-messages")
// ...
}
Parsing code should exist exclusively in XMLUtils
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing code should exist exclusively in
XMLUtils
.
@Electroid How should the implementation be handled for moving parsing to XMLUtils
. Should ObjectiveOption
be moved as well so we can reference it directly and have the method be something like:
public static Set<ObjectiveOption> parseObjectiveOptions(Element el, boolean def) {
// ...
}
Or were you thinking of a more generic parsing method? I feel like since this is a pretty specific use case it's better off residing inside of ObjectiveOptions
, similar to how we already have a KitParser
or ControlPointParser
. If you want I can extract the static parse method to a separate ObjectiveOptionParser
class, but seems unnecessary.
Any suggestions would be appreciated 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should just be a top-level
enum
.
The thing is "ShowOptions" is not an enum, it's a set of show flags. and it makes sense for them to be wrapped rather than having to pass around an ImmutableSet<ObjectiveOption>
Also, i have one major issue with naming it "objective option" and that is that it's an over-generalization, this is specifically for "show-*" toggles, not for any random boolean you want to put on an objective, otherwise we'd shove half of the objective xml into here
Maybe we can have it be called ShowOption
if you want to avoid confusion with actual flag objectives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for now, improvement is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjectiveOption
/ ObjectiveOptions
/ hasOption
are too generic of a name. What is "option" supposed to mean, anything you put on the xml is an "option" (the region? the material? the owning team?) but that isn't what this is about.
These are show-flags, makes sense @Electroid don't wanna call them "flags" for the confusion with the Flag objective but this needs to represent what it's about
Signed-off-by: applenick <applenick@users.noreply.github.com>
Signed-off-by: applenick <applenick@users.noreply.github.com>
Signed-off-by: applenick <applenick@users.noreply.github.com>
Expand visibility controls for objectives
This PR was inspired by a set of changes @Pablete1234 provided from his previous work back on the old PGM. Introduced are several new attributes that can be used when defining objectives in XML. These new show flags allow map authors to precisely control where and how objectives are displayed within a match. For example, you'll now be able to hide if a wool is shown on the scoreboard, or if a core broadcasts messages or not. This will hopefully provide mapmakers with greater flexibility to customize the user experience per-map.
Show Flags
show-messages
show-effects
show-info
show-sidebar
stats
The existing
show
attribute will now function as the default value for all the above flags, which is still by defaulttrue
.These show flags work for the following objective types:
XML Syntax
Feedback
As per usual, these changes have been tested and should all work as intended. If you have any suggestions/requests let me know and I'd be glad to look further into it 👍
Signed-off-by: applenick applenick@users.noreply.github.com