actually...looking at the code - neither is really correct. We should upgrade this to being an enum. Most everything just passes it around - there's like may be 10 references, and then its actual interpretation is in engine/src/gfx/mesh_gfx.cpp and all its meanings are defined there as well. We probably should upgrade the meanings to a shared header and just make it an enum which would give us a lot of safety checks on its usage as well.
Other locations:
That last one would probably be best to be converted to a std::map<enum, Pass> in the process.
The first one in techniques is basically loading XML files, parsing the values, and then loading them into the vector that is the last one; I could see that being potentially buggy since it relies on just an incrementing number. https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/engine/src/gfx/pass.cpp#L65-L110 would also need to get updated for using the new enum incrementally instead of the integer value.
So basics:
- at minimum the
size_t is correct because it is unsigned vs the int which is signed. Since this is currently an index into a vector and fairly unchecked at that having it unsigned is a good first step.
- ideally this would all get upgraded to utilize an enumeration and move away from a vector to a map; both have constant access after initialization but overall its safer and makes things more bounded. We could also error out when the parsePass/technique loading goes beyond something we recognize.
Some more research might be needed to ensure I caught all the places this references and that its not exposed to the Python side.
Originally posted by @BenjamenMeyer in #1670 (comment)
actually...looking at the code - neither is really correct. We should upgrade this to being an enum. Most everything just passes it around - there's like may be 10 references, and then its actual interpretation is in engine/src/gfx/mesh_gfx.cpp and all its meanings are defined there as well. We probably should upgrade the meanings to a shared header and just make it an enum which would give us a lot of safety checks on its usage as well.
Other locations:
That last one would probably be best to be converted to a
std::map<enum, Pass>in the process.The first one in techniques is basically loading XML files, parsing the values, and then loading them into the vector that is the last one; I could see that being potentially buggy since it relies on just an incrementing number. https://github.com/vegastrike/Vega-Strike-Engine-Source/blob/master/engine/src/gfx/pass.cpp#L65-L110 would also need to get updated for using the new enum incrementally instead of the integer value.
So basics:
size_tis correct because it is unsigned vs theintwhich is signed. Since this is currently an index into a vector and fairly unchecked at that having it unsigned is a good first step.Some more research might be needed to ensure I caught all the places this references and that its not exposed to the Python side.
Originally posted by @BenjamenMeyer in #1670 (comment)