-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat: add inventory screen titles #45
Conversation
"type": "UILabel", | ||
"id": "inventoryTitle", | ||
"family": "title", | ||
"text": "${engine:menu#category-inventory}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for using the translated string here. I noticed this only has an effect after restarting the game and created MovingBlocks/Terasology#4837 to track that.
import org.terasology.engine.logic.players.LocalPlayer; | ||
import org.terasology.nui.databinding.ReadOnlyBinding; | ||
import org.terasology.engine.registry.In; | ||
import org.terasology.engine.rendering.nui.CoreScreenLayer; | ||
import org.terasology.nui.widgets.UILabel; | ||
|
||
import java.util.Optional; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this import used?
import java.util.Optional; |
EntityRef characterEntity = localPlayer.getCharacterEntity(); | ||
CharacterComponent characterComponent = characterEntity.getComponent(CharacterComponent.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving this out of the get
might be fine, but we should be aware of the difference in semantics here.
I'm not 100% sure when the binding is evaluated, but I think this happens more often than the container screen is initialized.
I don't expect the characterEntity
to change during the life-time of this container screen, but I'm wondering whether we should fetch the characterComponent
each time rather than just once in the beginning.
DisplayNameComponent displayName = parentPrefab.getComponent(DisplayNameComponent.class); | ||
if (displayName != null) { | ||
return displayName.name; | ||
} else { | ||
return parentPrefab.getName(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you asked yesterday: This is how this would look with using Optional
:
DisplayNameComponent displayName = parentPrefab.getComponent(DisplayNameComponent.class); | |
if (displayName != null) { | |
return displayName.name; | |
} else { | |
return parentPrefab.getName(); | |
} | |
return Optional.ofNullable(parentPrefab.getComponent(DisplayNameComponent.class)) | |
.map(displayName -> displayName.name) | |
.orElse(parentPrefab.getName()); |
Since this is within a binding, I'm not sure the additional allocation of an Optional
might not be ideal, and the current code might indeed be the better solution here.
… into feat/add-inventory-screen-title
In #45 the character entity was cached outside of the binding evaluation. However, it in multiplayer games it can happen that it is `null`, and therefore opening container screens on a connected client fails. This is fixed by retrieving the character entity with each binding evaluation via a common helper method `getPredictedInteractionTarget`.
In #45 the character entity was cached outside of the binding evaluation. However, it in multiplayer games it can happen that it is `null`, and therefore opening container screens on a connected client fails. This is fixed by retrieving the character entity with each binding evaluation via a common helper method `getPredictedInteractionTarget`.
This PR adds a (translated) title to the inventory screen.
In itself this doesn't do much yet and might seem unnecessary. However, especially when using item blocks that have an inventory (chests, assembly tables, etc.), it easily becomes confusing, which inventory is the own and which is the inventory of the block being interacted with. Hence, I added this for container inventory as well.
If the container parent prefab does not have a
DisplayNameComponent
, the prefab name will be shown, for instanceCoreAdvancedAssets:chest
. Terasology/CoreAdvancedAssets#5 adds aDisplayNameComponent
to the chest prefab.This should result in a first improvement and can be further extended by applying similar changes to items with inventory slots, for instance elements of Josharias' Survival.