numbers aren't magic #2976
numbers aren't magic #2976
Conversation
There was a problem hiding this comment.
Overall, I am onboard. I wonder if we should add a little more specificity to the enum names. I'm thinking similar to how our randomizer options are all RO_XXX, we could for example have all the enhancement options here be prefixed with EO_XXX
Granted there are more enums in this PR than just this file, but just throwing out ideas to help with the namespace of things.
There was a problem hiding this comment.
i'm down to set the pattern of EO_ for things in this file, not sure how best to handle the things beyond this one though (maybe those can be a future problem if we can't think of something good?)
There was a problem hiding this comment.
I disagree, I think these are fine as is since they are already being prefaced with the option they are enumerating for. I think the initial EO might just add extra noise to autocomplete stuff.
There was a problem hiding this comment.
My concern is less with identifying the option name, and more so with "specificity" in the include/header space to help avoid collisions, especially as this file will grow when more enhancements are added. Again just a suggestion, I won't die on the hill over it.
(The pattern is used both in the original game code for enums, and some of our own existing enums)
There was a problem hiding this comment.
Were the enums actually in the original game code or is that decomp documentation? Anyway, I don't disagree with the specificity and pattern points, I just think that having the option name in the enums is enough to satisfy those points without the EO prefix. I also won't die on this hill though, adding EO is fine with me if bria thinks it's better.
There was a problem hiding this comment.
i don't have strong feelings one way or the other, i just want to not have magic numbers
There was a problem hiding this comment.
I honestly don't think the prefix is necessary, either. If you really want to do specificity, then you could separate the enums into separate headers per group, but I also think the name of the option being in the enum value names is enough distinction between the enums themselves. Maybe in the enum name instead of the value names?
There was a problem hiding this comment.
sounds like there are more people in the "don't add the EO prefix" camp at the moment. we can revisit that decision later if we want to, i'm happy without it for now
renamed everything except the cvar itself at this point (waiting for versioned configs for that)
did a big search for all the comboboxes using magic numbers and threw some enums in them
i also found a typo and fixed that in a few places
i also did a little logic cleanup in a couple places to make things read more clearly with the enum values
very open to naming suggestions and ideas about where these new enums should live
Build Artifacts