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

Status screen rework #51

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Status screen rework #51

wants to merge 7 commits into from

Conversation

syntaxi
Copy link
Member

@syntaxi syntaxi commented Nov 8, 2018

This PR reworks the status screen to be less of a pile of code smells.

In particular it actually integrates it into the inventory and allows for a variable number of effects.

To Test

Simply drink some potions and open up the inventory, there should be a screen on the right

Outstanding

Make potion effects automatically load from the player.

Copy link
Contributor

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

I'm not really sure that the inventory screen override is the right thing to do, and how it behaves with other modules. For instance, JS also has a different inventory screen - could those two modules be used together?



@RegisterSystem(RegisterMode.CLIENT)
public class PotionStatusUISystem extends BaseComponentSystem {

private static final String PERIODIC_ACTION_ID = "PotionStatusScreenUI";
private static final String TARGET_SCREEN_NAME = "Core:InventoryScreen";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static final String TARGET_SCREEN_NAME = "Core:InventoryScreen";
private static final String TARGET_SCREEN_NAME = "Inventory:InventoryScreen";

Copy link
Contributor

Choose a reason for hiding this comment

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

weirdly, it doesn't work for me locally if I use Inventory:InventoryScreen ... what is this referring to? 🤔


public PotionStatusWidget() {
effects.setFillVerticalSpace(false);
effects.addWidget(new UILabel("Potion Effects:"));
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always show the label, even if no potions are active. This could be improved later on.

Copy link
Contributor

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

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

Was a bit too fast in approving this 🙈 after merging with master it no longer displays the potion effect ...

@jellysnake if you find the time, please have a look whether this can easily be recovered....

@jdrueckert jdrueckert changed the base branch from master to develop May 21, 2020 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants