-
Notifications
You must be signed in to change notification settings - Fork 8
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 shop-screen #22
Conversation
/** | ||
* The cost of buying the item. | ||
* If left blank on the prefab, the value component will be used (if it exists). | ||
*/ | ||
public int cost = -1; |
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.
The default value here is -1, but what does that mean? Does the player get money back when they buy it? And why is -1 a better default value than 0?
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.
And why do we need this at all? This is just a shortcut to avoid having both Purchasable
and Value
on an entity, right?
If possible I'd rather have one "marker component" without any fields instead of tracking the same thing in two places.
/** | ||
* Gives the entity to the target entity | ||
*/ | ||
@ServerEvent | ||
public class GiveItemTypeEvent implements Event { |
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.
Given that the name is so similar to GiveItemEvent
I think we should explain the difference between the two here, and also why we need this event in addition.
public class GiveItemTypeEvent implements Event { | ||
Prefab targetPrefab; | ||
|
||
public GiveItemTypeEvent() { |
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.
It might be better to make this constructor package private as it is only meant for internal usage for deserialization, but this event does not make much sense with a null
prefab.
|
||
@ReceiveEvent | ||
public void onGiveItemToPlayer(GiveItemTypeEvent event, EntityRef entity) { | ||
if (event.getTargetPrefab() != null) { |
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.
That's a safe guard as I would have put it here, but given the recent discussion on World::setBlock
I'm wondering what @keturn would do here?
As this event with a null
prefab is most likely a bug we should at least log a WARN message if this case occurs.
If we want to be sure that we notice that the case occurred we should probably even throw an exception here. I'm not sure whether we are able to figure out the original sender of the event with an exception here, but I hope that the stacktrace would reveal that information.
public void onGiveItemToPlayer(GiveItemTypeEvent event, EntityRef entity) { | ||
if (event.getTargetPrefab() != null) { | ||
EntityRef item = entityManager.create(event.getTargetPrefab()); | ||
if (walletAuthoritySystem.isValidTransaction(entity, -item.getComponent(PurchasableComponent.class).cost)) { |
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.
Can't we get the component from the prefab even before creating the final entity? If the player cannot purchase an item we should probably not create it.
|
||
/** | ||
* Wrapper widget used in the shop screen. | ||
* Displays a separate widget whilst capturing user interaction with it. |
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.
The main use case for this is for tooltips, it seems. Can you explain why this is necessary in more detail?
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.
I looked over the prior commits I made and, although I don't recall all the details, it is likely to do with the way user interaction is handled.
We can't just use the ItemIcon
widget that this contains, because we cannot assign a listener to it
We need to assign a listener to it, because clicking on this widget indicated that this item is selected.
This is visible from the line in ShopScreen#addBlocks
wrapper.setListener(widget -> handleBlockSelected(block));
and the line in ShopScreen#addItems
wrapper.setListener(widget -> handlePrefabSelected(item));
The listener indicates which shop entry was selected, as well as calling the handler for the specific type, item vs block.
If this is no longer how the code works, and selection is not done by clicking an item in the ship screen, the this is likely unneeded. However, as far as I am aware, this is not the case and as such, the wrapper is needed.
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.
Thanks for the explanation!
* @return The price of the ware. | ||
*/ | ||
public static int getWareCost(ComponentContainer ware) { | ||
int cost = ware.getComponent(PurchasableComponent.class).cost; |
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.
I'd add a check using Guava preconditions (checkArgument
) that ware
actually is Purchasable
. Without this check this method will just fail with a NPE.
purchasableBlocks = blocks.stream() | ||
.map(blockManager::getBlockFamily) | ||
.map(BlockFamily::getArchetypeBlock) | ||
.filter(block -> block.getPrefab().isPresent()) | ||
.filter(block -> block.getPrefab().get().hasComponent(PurchasableComponent.class)) | ||
.collect(Collectors.toSet()); |
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.
Ah, so all blocks with a Purchasable
component appear in the shop - that actually sounds reasonable 👍 (at least for the start).
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.
That is how I designed it in gooey defense in order to save time. I could easily see alternatives used, especially if multiple shops are to be used in parallel. (eg, metal renagades). In this situation, perhaps a URI for each shop which is referenced inside the purchasable component?
if (character.getComponent(CurrencyStorageComponent.class).amount - getWareCost(blockItem) >= 0) { | ||
character.send(new WalletTransactionEvent(-getWareCost(blockItem))); | ||
inventoryManager.giveItem(character, character, blockItem); | ||
} |
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.
Why do we do the transaction manually here instead of using the new event?
Do I see it correctly, that this now no longer has a dependency on ST? |
Yup |
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.
We now have code style annotations in the "Files changed" tab. Please have a look and try to resolve them. Resolving issues in files that your PR doesn't touch is optional (yet appreciated 😅)
public static int getWareCost(ComponentContainer ware) { | ||
Preconditions.checkNotNull(ware.getComponent(ValueComponent.class), "Component Container does not contain a Value Component"); | ||
return ware.getComponent(ValueComponent.class).value; | ||
} |
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.
Why can we drop the negative costs check that was present here in the GD implementation?
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.
before if the PurchasableComponent was -1 that meant that the price was stored in the ValueComponent but over here instead we are solely relying on the ValueComponent for the price.
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.
Makes me wonder why we differentiate between the two in the first place and why GD even allows for the price to be in either.
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.
I'd assume this has "historical reasons" and our usual reluctance to use and/or modify existing concepts 🤔
if (walletAuthoritySystem.isValidTransaction(entity, -item.getComponent(ValueComponent.class).value)) { | ||
entity.send(new WalletTransactionEvent(-item.getComponent(ValueComponent.class).value)); |
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.
Why does the ItemAuthoritySystem
check the transaction and send the WalletTransactionEvent
and not the ShopManager
?
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 the ShopManager is not an authoritative system and these checks are better used in Authoritative Systems.
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.
Is there a reason, why the ShopManager should stay a non-authoritative system?
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.
This is so that the ShopScreen which is accessible by all players can access the ShopManager.
/** | ||
* Trigger event that sends the required item (as a string/prefab) to the server. | ||
* It then creates the item entity and gives the item on the server. | ||
*/ |
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.
I guess the difference to GiveItem
here is that the item is not actually "moved" but only a copy of it? I think it would help to actively state the difference here, including a cross-reference.
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.
My understanding is that this event is a trigger, to create and then give an item to the player.
if (walletAuthoritySystem.isValidTransaction(entity, -blockItem.getComponent(ValueComponent.class).value)) { | ||
entity.send(new WalletTransactionEvent(-blockItem.getComponent(ValueComponent.class).value)); | ||
blockItem.send(new GiveItemEvent(entity)); | ||
} |
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.
It seems like maybe we could extract some code shared between the prefab and the block case here.
* Displays a separate widget whilst capturing user interaction with it. | ||
* When the icon corresponding to the item is clicked, a tooltip containing the item's name and additional information is displayed. | ||
* <p> | ||
* All size values etc etc are determined by the content widget. |
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.
* All size values etc etc are determined by the content widget. | |
* All size values etc are determined by the content widget. |
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.
Minor comments and clarifying a question from skaldanar
import org.terasology.module.inventory.systems.InventoryAuthoritySystem; | ||
|
||
@RegisterSystem(RegisterMode.AUTHORITY) | ||
public class ItemAuthoritySystem extends BaseComponentSystem { |
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.
I believe that it may be wise to remove this system altogether
Like it was pointed out, it makes more sense for the shop system to check the value, and furthermore I don't see why any of this code has to be done in a separate system.
If we are firing an event, which is only intended to be used for calling this system, then the two should be combined.
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.
The main reason for this system is because the GiveItemEvent seems to work mainly on Authoritative Systems and this becomes necessary for multiplayer, and after discussing this with @skaldarnar and @jdrueckert they suggested creating a separate system so that even MR can eventually use this. (MR also makes use of a separate system for this in a similar manner if I am not mistaken)
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.
I tried to integrate that (as authority event handler) into the ShopManager
in 969e482
|
||
/** | ||
* Wrapper widget used in the shop screen. | ||
* Displays a separate widget whilst capturing user interaction with it. |
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.
I looked over the prior commits I made and, although I don't recall all the details, it is likely to do with the way user interaction is handled.
We can't just use the ItemIcon
widget that this contains, because we cannot assign a listener to it
We need to assign a listener to it, because clicking on this widget indicated that this item is selected.
This is visible from the line in ShopScreen#addBlocks
wrapper.setListener(widget -> handleBlockSelected(block));
and the line in ShopScreen#addItems
wrapper.setListener(widget -> handlePrefabSelected(item));
The listener indicates which shop entry was selected, as well as calling the handler for the specific type, item vs block.
If this is no longer how the code works, and selection is not done by clicking an item in the ship screen, the this is likely unneeded. However, as far as I am aware, this is not the case and as such, the wrapper is needed.
/** | ||
* Trigger event that sends the description of the required item (as a string/prefab) to the server. | ||
* It then creates the item entity using the description and gives the item on the server. | ||
*/ |
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.
Seems like there's some comments that got lost here: I think this documentation still does not clearly distinguish GiveItemType
from the "normal" GiveItem
|
||
@Override | ||
public void postBegin() { | ||
BlockExplorer blockExplorer = new BlockExplorer(assetManager); |
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.
code style nitpick: move this closer to where it is used, i.e., after the purchasableItems
statement.
* Indicates that the item can be bought and thus will be available in the shop | ||
*/ | ||
@AddToBlockBasedItem | ||
public class PurchasableComponent implements Component<PurchasableComponent> { |
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.
public class PurchasableComponent implements Component<PurchasableComponent> { | |
public class PurchasableComponent extends EmptyComponent<PurchasableComponent> { |
@Override | ||
public void copyFrom(PurchasableComponent other) { | ||
} |
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.
@Override | |
public void copyFrom(PurchasableComponent other) { | |
} |
|
||
private Logger logger = LoggerFactory.getLogger(CurrencyStorageHandler.class); | ||
private static final Logger logger = LoggerFactory.getLogger(CurrencyStorageHandler.class); | ||
private final String currency = "currency"; |
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.
Why change this this way? It looks like it was intended to be a constant value, so just making it static as well would be a better fix?
private final String currency = "currency"; | |
private static final String CURRENCY = "currency"; |
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.
I was trying to resolve some of the suggestions given.
|
||
/** | ||
* Wrapper widget used in the shop screen. | ||
* Displays a separate widget whilst capturing user interaction with it. |
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.
Thanks for the explanation!
* All size values etc are determined by the content widget. | ||
*/ |
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.
I'd like to add @syntaxi 's explanation (https://github.com/Terasology/Economy/pull/22/files#r696603417) as in-code comment here:
* All size values etc are determined by the content widget. | |
*/ | |
* All size values etc are determined by the content widget. | |
* <p> | |
* NOTE: We can't just use the {@code ItemIcon} widget that this contains, because we cannot assign a listener to it. | |
* We need to assign a listener to it, because clicking on this widget indicated that this item is selected. | |
* This is visible from the line in ShopScreen#addBlocks | |
* <pre> | |
* wrapper.setListener(widget -> handleBlockSelected(block)); | |
* </pre> | |
* and the line in ShopScreen#addItems | |
* <pre> | |
* wrapper.setListener(widget -> handlePrefabSelected(item)); | |
* </pre> | |
* The listener indicates which shop entry was selected, as well as calling the handler for the specific type, item vs block. | |
*/ |
I think even if a system is registered ALWAYS, we can have a `netFilter` annotated to specific event handlers, like so: ``` @ReceiveEvent(netFilter = RegisterMode.AUTHORITY) ``` This allows us to have code running only on the authority, e.g., for handling purchase of items, while avoiding a completely different system.
These changes allows the items to be bought in multiplayer using the shop screen.