Consistent semantics for powerups and bonuses #3437
Conversation
|
Maybe instead of PlayerBonusType, we could have just PlayerType and the values could be something like SMALL_TUX, BIG_TUX, FIRE_TUX, etc.. Just keep in mind that the legacy names should be still usable from the squirrel interface, so it doesn't break old levels. Just an idea. |
|
Yeah I think I would prefer names like FIRE_TUX or ICE_TUX instead of BONUS_FIRE etc., but at the same time if backwards compatibility means that they are no better than aliases for the current versions, it might just make things more confusing rather than less. And it would require touching a lot of files. But at the same time I can see where it could be worth it to prevent things like Oh and between |
|
Actually, thinking about it a little more, we could take this a step farther to gain some interesting new functionality. If we add a PlayerStatus::active_powerup attribute, this could reuse the powerup enum directly instead of/in addition to PlayerBonusType. This could be useful in the case of the retro/reskined powerups, allowing for e.g. a coffee sprite in the item pocket, or coffee bullets with powerup type Or it's probably fine as-is; I think the coffee bullet idea has at least convinced me that PlayerBonusType is being used correctly in the places it's still left. |
swagtoy
left a comment
swagtoy
left a comment
There was a problem hiding this comment.
I like what I'm seeing, but I really wonder if we should move the PlayerBonusType enum into the Player class itself. Have not tested this PR, just a code review
src/object/flower.hpp
Outdated
| BonusType type; | ||
| PowerUp::Type type; | ||
| PlayerBonusType bonus; | ||
| SpritePtr sprite; | ||
| Flip flip; |
There was a problem hiding this comment.
Really, these should be prefixed m_, that way you can avoid doing _type as a function
src/object/pocketpowerup.cpp
Outdated
| physic.set_velocity_y(-325.f); | ||
| physic.set_gravity_modifier(0.4f); |
There was a problem hiding this comment.
I acknowledge that you moved the code into this class, but I think you should put these constants in their own variables, maybe as static constexpr in the class definition itself so others could access them (or just put them locally in the file; up to you)
| /// PowerUp that flings itself upwards | ||
| /// can't be collected right away. |
There was a problem hiding this comment.
nit: aggressive line width here must've cut this comment in half
There was a problem hiding this comment.
I think this was intentionally two lines, like two sentences?
| enum PlayerBonusType { | ||
| BONUS_NONE = 0, /*!< @description No bonus. */ | ||
| BONUS_GROWUP, /*!< @description Growup (a.k.a. egg) bonus. */ | ||
| BONUS_FIRE, /*!< @description Fire bonus. */ | ||
| BONUS_ICE, /*!< @description Ice bonus. */ | ||
| BONUS_AIR, /*!< @description Air bonus. */ | ||
| BONUS_EARTH /*!< @description Earth bonus. */ | ||
| }; |
There was a problem hiding this comment.
a part of me wants to say; these should really be in the Player class itself if they are only for the player, so like Player::BonusType
There was a problem hiding this comment.
I think moving it to player.hpp would create more circular include dependencies, which I guess can be dealt with in other ways, but it seems fine where it is? like right now it's being used for the type of bullets that tux fires which is probably fine as long as they don't become more complex.
4 participants
Powerups, bonusblocks, and friends now use
PowerUp::Typewhere possible.BonusTypeis renamed toPlayerBonusTypeto clarify its usage, i.e., to enumerate Tux's (or other future players?) possible states.Function signatures are changed from
intsto the appropriateenumtype where possible.PocketPowerUpis moved fromplayer_statusto its own file insrc/objectto eliminate circular include dependencies betweenpowerup.hppandplayer_status.hpp.Hopefully this will make working and adding new powerups easier in the future.
Let me know if you see any mistakes or have any suggestions!