Repository navigation
added objectids - #1348
added objectids#1348absoIute wants to merge 8 commits into
Conversation
|
Part of me honestly wonders whether this really should be included in Enums.hpp, as it's a relatively large 4k line file and amount of mods interacting with objects is small |
|
i doubt it'd make any meaningful difference to compile times considering the content |
|
this is fine in theory, i just feel like this could be reduced to the ones people actually focus on (legacy color triggers, invisible blocks introduced in 1.6, sawblade objects [since those dont have a more granular game object type besides yes, object type enums exist for jump pads/orbs/portals/etc, but enabling "No Touch" changes their object type to |
adding all the triggers ever to this list as well, since no one wants to memorize those triggers by numeric object ID alone |
|
that's what i was originally suggesting, but who are we to dictate what objects people are allowed to make mods for |
imo still unnecessary cost, but I wouldn't mind it included in
|
|
Also consider the cost on CI |
|
tru dat |
|
i'm still unsure about this being a bunch of constexprs instead of an enum. if this is merged what would the case for m_objectID to still be an int? comparisons & storage would obviously move to the enum, and there's not really a reason to be doing math operations on them (apart from range comparisons? in which case we could provide a <=> overload) |
|
To verify affects on compile times, I benchmarked different approaches with hyperfine (5 warmup, 20 benchmarked runs): Current approach (ObjectIDs.hpp separate include): Include ObjectIDs.hpp in Enums.hpp directly: Include ObjectIDs.hpp in Enums.hpp directly, and turn it into an enum class: Specs: As I understand, there is no discernible affect on compile times between approaches. Using the enum class in members would improve readability of inlines and would encourage all devs to use the enum over magic numbers, which seems like a no-brainer. |
|
note: as this is a breaking change now, it will have to wait for v6 / 2.209. until then, more debate could be had for which option is better (namespace vs enum) |
|
@absoIute would love to see those tests repeated on the specific github runners used for github actions for a more realistic comparison |
|
i really don't think benchmarking github runners would be that reliable |
7cc2753 to
6f7b5b3
Compare
4feae1a to
d40eda2
Compare
|
i'm voting for the |
|
while I do think the issue of casting is overstated (from my experience functions that require an |
018259b to
b0f89dc
Compare
with help from alpha's csv + @Nwo5-trg