Skip to content
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

Item Pocket #3070

Merged
merged 37 commits into from
Nov 21, 2024
Merged

Item Pocket #3070

merged 37 commits into from
Nov 21, 2024

Conversation

MatusGuy
Copy link
Member

@MatusGuy MatusGuy commented Oct 5, 2024

The Item Pocket allows you to save a powerup for later use. If you collect a flower while having a bonus greater than GROWUP_BONUS, that flower gets equipped and the old flower gets stored in the top left corner of the screen. Now, the player can press the new ITEM control (usually Select/Back or Left Shift) to use the stored flower by throwing it up and catching it.

This pull request replaces the powerup stacking feature because this new solution is much more balanced. It also limits the amount of concurrent players to 4.

@MatusGuy MatusGuy added type:idea status:in-progress Progress has been done, but more is intended be done category:code labels Oct 5, 2024
@tobbi
Copy link
Member

tobbi commented Oct 5, 2024

Please fix initialization order:

/Users/runner/work/supertux/supertux/src/supertux/player_status.cpp:38:3: error: field 'bonus' will be initialized after field 'm_item_pockets' [-Werror,-Wreorder-ctor]
  bonus(num_players),
  ^~~~~~~~~~~~~~~~~~
  coins(START_COINS)

@tobbi
Copy link
Member

tobbi commented Oct 5, 2024

Oh, right, this is a draft, so my comments were maybe a bit premature.

@tobbi
Copy link
Member

tobbi commented Oct 6, 2024

/home/runner/work/supertux/supertux/src/badguy/boss.cpp:76:70: error: implicit conversion from 'int' to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion]
    float startpos = (context.get_width() - (m_hud_head->get_width() * m_max_lives)) / 2;

@swagtoy
Copy link

swagtoy commented Oct 11, 2024

Has the ability to disable the itembox been discussed?

@bruhmoent
Copy link
Member

Has the ability to disable the itembox been discussed?

What do you mean by disabling it...? It's a core gameplay feature

@swagtoy
Copy link

swagtoy commented Oct 12, 2024

What do you mean by disabling it...? It's a core gameplay feature

Pardon? I meant that some level designers may want to disable it for themselves (or per world). I am curious on the consensus. I personally wouldnt enable it for my worlds

@MatusGuy MatusGuy marked this pull request as draft October 27, 2024 20:24
@MatusGuy MatusGuy marked this pull request as ready for review October 29, 2024 17:44
Copy link
Member

@Vankata453 Vankata453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, amazing work on this PR!

Encountered a bug: The "Add Player" option in "Multiplayer Settings" does nothing.

Additionally, I believe that launching the powerup from the item pocket to the side (like a Tuxdoll), perhaps to the opposite side of where Tux is facing, would make utilizing the powerup from the item pocket require more concentration and consideration of where to utilize, since the player would also have to catch it, instead of waiting for it in the same spot.

I am also personally not a fan of the blinking effect that launched pocket powerups have. Maybe we could do without it?

src/control/game_controller_manager.cpp Outdated Show resolved Hide resolved
src/control/joystick_manager.cpp Outdated Show resolved Hide resolved
src/supertux/player_status.cpp Outdated Show resolved Hide resolved
src/supertux/player_status.cpp Outdated Show resolved Hide resolved
src/supertux/player_status.cpp Outdated Show resolved Hide resolved
src/supertux/player_status.hpp Outdated Show resolved Hide resolved
src/supertux/player_status.hpp Outdated Show resolved Hide resolved
src/supertux/player_status_hud.cpp Outdated Show resolved Hide resolved
src/supertux/sector.cpp Outdated Show resolved Hide resolved
src/supertux/sector.hpp Outdated Show resolved Hide resolved
@MatusGuy
Copy link
Member Author

The blinking effect indicates that you can't pick it up yet. This is a mechanism that prevents immediately grabbing the object upon jumping.

Also, I think the general consensus is that it shouldn't move in x axis whatsoever but I might be wrong so I'll leave this up for discussion.

@MatusGuy
Copy link
Member Author

Can you check if power up stacking was truly removed in case I missed something?

Copy link
Member

@Vankata453 Vankata453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look at the comments below, as well as the following:

  • multiplayer_player_menu.cpp:118 uses the same code as GameSession::on_player_added so it can be replaced with a call to that function.
  • GameSession::on_player_removed shouldn't remove the player from PlayerStatus, since that will removed all the data for that player, when they may reconnect.
  • The "Add Player" bug from "Multiplayer Settings", as mentioned in the previous comment, is still present.

src/squirrel/supertux_api.cpp Outdated Show resolved Hide resolved
src/squirrel/supertux_api.cpp Outdated Show resolved Hide resolved
src/squirrel/supertux_api.cpp Outdated Show resolved Hide resolved
Copy link
Member

@bruhmoent bruhmoent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work - reviewed both the code and the in-game functionality

@Frostwithasideofsalt
Copy link
Member

image
seems theres a bug with the item pocket that makes it so theres multiple item pockets when theres only one player

Copy link
Member

@Frostwithasideofsalt Frostwithasideofsalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zero twerking found here, bucko! only working.

@Frostwithasideofsalt
Copy link
Member

being that this seems to be done, could it be merged?

Copy link
Member

@Vankata453 Vankata453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-wise looks good now!

Copy link
Member

@weluvgoatz weluvgoatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:code status:in-progress Progress has been done, but more is intended be done type:idea
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants