feat: add shop-screen #22

ahv15 · 2021-07-15T21:35:55Z

These changes allows the items to be bought in multiplayer using the shop screen.

skaldarnar · 2021-07-26T17:35:47Z

src/main/java/org/terasology/economy/components/PurchasableComponent.java

+    /**
+     * 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;


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?

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.

skaldarnar · 2021-07-26T17:39:07Z

src/main/java/org/terasology/economy/events/GiveItemTypeEvent.java

+/**
+ * Gives the entity to the target entity
+ */
+@ServerEvent
+public class GiveItemTypeEvent implements Event {


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.

skaldarnar · 2021-07-26T17:40:15Z

src/main/java/org/terasology/economy/events/GiveItemTypeEvent.java

+public class GiveItemTypeEvent implements Event {
+    Prefab targetPrefab;
+
+    public GiveItemTypeEvent() {


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.

skaldarnar · 2021-07-26T17:52:46Z

src/main/java/org/terasology/economy/systems/ItemAuthoritySystem.java

+
+    @ReceiveEvent
+    public void onGiveItemToPlayer(GiveItemTypeEvent event, EntityRef entity) {
+        if (event.getTargetPrefab() != null) {


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.

skaldarnar · 2021-07-26T17:53:58Z

src/main/java/org/terasology/economy/systems/ItemAuthoritySystem.java

+    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)) {


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.

skaldarnar · 2021-07-26T17:56:26Z

src/main/java/org/terasology/economy/ui/UIInteractionWrapper.java

+
+/**
+ * Wrapper widget used in the shop screen.
+ * Displays a separate widget whilst capturing user interaction with it.


The main use case for this is for tooltips, it seems. Can you explain why this is necessary in more detail?

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.

Thanks for the explanation!

src/main/java/org/terasology/economy/ui/ShopScreen.java

skaldarnar · 2021-07-26T18:38:55Z

src/main/java/org/terasology/economy/ShopManager.java

+     * @return The price of the ware.
+     */
+    public static int getWareCost(ComponentContainer ware) {
+        int cost = ware.getComponent(PurchasableComponent.class).cost;


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.

skaldarnar · 2021-07-26T18:40:18Z

src/main/java/org/terasology/economy/ShopManager.java

+        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());


Ah, so all blocks with a Purchasable component appear in the shop - that actually sounds reasonable 👍 (at least for the start).

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?

skaldarnar · 2021-07-26T18:41:31Z

src/main/java/org/terasology/economy/ShopManager.java

+        if (character.getComponent(CurrencyStorageComponent.class).amount - getWareCost(blockItem) >= 0) {
+            character.send(new WalletTransactionEvent(-getWareCost(blockItem)));
+            inventoryManager.giveItem(character, character, blockItem);
+        }


Why do we do the transaction manually here instead of using the new event?

jdrueckert · 2021-08-24T20:59:35Z

Do I see it correctly, that this now no longer has a dependency on ST?

ahv15 · 2021-08-24T21:30:11Z

Do I see it correctly, that this now no longer has a dependency on ST?

Yup

jdrueckert

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 😅)

jdrueckert · 2021-08-25T07:12:00Z

src/main/java/org/terasology/economy/ShopManager.java

+    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;
+    }


Why can we drop the negative costs check that was present here in the GD implementation?

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.

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.

I'd assume this has "historical reasons" and our usual reluctance to use and/or modify existing concepts 🤔

jdrueckert · 2021-08-25T07:15:29Z

src/main/java/org/terasology/economy/systems/ItemAuthoritySystem.java

+            if (walletAuthoritySystem.isValidTransaction(entity, -item.getComponent(ValueComponent.class).value)) {
+                entity.send(new WalletTransactionEvent(-item.getComponent(ValueComponent.class).value));


Why does the ItemAuthoritySystem check the transaction and send the WalletTransactionEvent and not the ShopManager?

Because the ShopManager is not an authoritative system and these checks are better used in Authoritative Systems.

Is there a reason, why the ShopManager should stay a non-authoritative system?

This is so that the ShopScreen which is accessible by all players can access the ShopManager.

jdrueckert · 2021-08-25T07:18:08Z

src/main/java/org/terasology/economy/events/GiveItemTypeEvent.java

+/**
+ * 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.
+ */


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.

My understanding is that this event is a trigger, to create and then give an item to the player.

jdrueckert · 2021-08-25T07:19:33Z

src/main/java/org/terasology/economy/systems/ItemAuthoritySystem.java

+            if (walletAuthoritySystem.isValidTransaction(entity, -blockItem.getComponent(ValueComponent.class).value)) {
+                entity.send(new WalletTransactionEvent(-blockItem.getComponent(ValueComponent.class).value));
+                blockItem.send(new GiveItemEvent(entity));
+            }


It seems like maybe we could extract some code shared between the prefab and the block case here.

jdrueckert · 2021-08-25T07:22:27Z

src/main/java/org/terasology/economy/ui/UIInteractionWrapper.java

+ * 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.


Suggested change

* All size values etc etc are determined by the content widget.

* All size values etc are determined by the content widget.

syntaxi

Minor comments and clarifying a question from skaldanar

syntaxi · 2021-08-26T12:38:35Z

src/main/java/org/terasology/economy/systems/ItemAuthoritySystem.java

+import org.terasology.module.inventory.systems.InventoryAuthoritySystem;
+
+@RegisterSystem(RegisterMode.AUTHORITY)
+public class ItemAuthoritySystem extends BaseComponentSystem {


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.

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)

I tried to integrate that (as authority event handler) into the ShopManager in 969e482

syntaxi · 2021-08-26T12:57:52Z

src/main/java/org/terasology/economy/ui/UIInteractionWrapper.java

+
+/**
+ * Wrapper widget used in the shop screen.
+ * Displays a separate widget whilst capturing user interaction with it.


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.

jdrueckert · 2021-08-28T14:25:29Z

src/main/java/org/terasology/economy/events/GiveItemTypeEvent.java

+/**
+ * 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.
+ */


Seems like there's some comments that got lost here: I think this documentation still does not clearly distinguish GiveItemType from the "normal" GiveItem

skaldarnar · 2021-08-28T14:34:17Z

src/main/java/org/terasology/economy/ShopManager.java

+
+    @Override
+    public void postBegin() {
+        BlockExplorer blockExplorer = new BlockExplorer(assetManager);


code style nitpick: move this closer to where it is used, i.e., after the purchasableItems statement.

skaldarnar · 2021-08-28T14:38:07Z

src/main/java/org/terasology/economy/components/PurchasableComponent.java

+ * Indicates that the item can be bought and thus will be available in the shop
+ */
+@AddToBlockBasedItem
+public class PurchasableComponent implements Component<PurchasableComponent> {


Suggested change

public class PurchasableComponent implements Component<PurchasableComponent> {

public class PurchasableComponent extends EmptyComponent<PurchasableComponent> {

skaldarnar · 2021-08-28T14:38:16Z

src/main/java/org/terasology/economy/components/PurchasableComponent.java

+    @Override
+    public void copyFrom(PurchasableComponent other) {
+    }


Suggested change

@Override

public void copyFrom(PurchasableComponent other) {

}

skaldarnar · 2021-08-28T14:40:25Z

src/main/java/org/terasology/economy/handler/CurrencyStorageHandler.java


-    private Logger logger = LoggerFactory.getLogger(CurrencyStorageHandler.class);
+    private static final Logger logger = LoggerFactory.getLogger(CurrencyStorageHandler.class);
+    private final String currency = "currency";


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?

Suggested change

private final String currency = "currency";

private static final String CURRENCY = "currency";

I was trying to resolve some of the suggestions given.

skaldarnar · 2021-08-28T14:46:31Z

src/main/java/org/terasology/economy/ui/UIInteractionWrapper.java

+
+/**
+ * Wrapper widget used in the shop screen.
+ * Displays a separate widget whilst capturing user interaction with it.


Thanks for the explanation!

skaldarnar · 2021-08-28T14:50:37Z

src/main/java/org/terasology/economy/ui/UIInteractionWrapper.java

+ * All size values etc are determined by the content widget.
+ */


I'd like to add @syntaxi 's explanation (https://github.com/Terasology/Economy/pull/22/files#r696603417) as in-code comment here:

Suggested change

* 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.

ahv15 requested review from jdrueckert and skaldarnar July 15, 2021 21:44

This was referenced Jul 15, 2021

feat: implementation for economy Terasology/LightAndShadow#228

Merged

refactor!: move shop screen to Economy module Terasology/GooeyDefence#68

Merged

ahv15 marked this pull request as draft July 17, 2021 13:40

ahv15 marked this pull request as ready for review July 23, 2021 11:39

skaldarnar requested a review from syntaxi July 26, 2021 17:33

skaldarnar reviewed Jul 26, 2021

View reviewed changes

ahv15 requested a review from skaldarnar July 28, 2021 16:38

revert changes

21435a7

ahv15 force-pushed the shop-screen branch from bd12967 to 21435a7 Compare August 24, 2021 17:33

jdrueckert requested changes Aug 25, 2021

View reviewed changes

syntaxi reviewed Aug 26, 2021

View reviewed changes

make requested changes

315e6ab

ahv15 requested review from jdrueckert and syntaxi August 27, 2021 14:31

ahv15 added 3 commits August 27, 2021 20:18

fix merge conflicts

eb70ac9

update

f726b32

make-changes

8c3cea7

jdrueckert reviewed Aug 28, 2021

View reviewed changes

skaldarnar reviewed Aug 28, 2021

View reviewed changes

ahv15 added 2 commits August 28, 2021 22:41

make changes

d4de0cb

make changes

5f02afc

ahv15 requested review from skaldarnar and jdrueckert August 28, 2021 17:24

skaldarnar and others added 3 commits August 28, 2021 19:35

refactor: rename GiveItemTypeEvent >>> PurchaseItemEvent

0ea2096

Merge branch 'develop' into shop-screen

bef68c5

fix: add missing imports

83f0131

jdrueckert approved these changes Oct 9, 2021

View reviewed changes

jdrueckert merged commit c406a33 into Terasology:develop Oct 9, 2021

		if (walletAuthoritySystem.isValidTransaction(entity, -item.getComponent(ValueComponent.class).value)) {
		entity.send(new WalletTransactionEvent(-item.getComponent(ValueComponent.class).value));

	* All size values etc etc are determined by the content widget.
	* All size values etc are determined by the content widget.

	public class PurchasableComponent implements Component<PurchasableComponent> {
	public class PurchasableComponent extends EmptyComponent<PurchasableComponent> {

	@Override
	public void copyFrom(PurchasableComponent other) {
	}

	private final String currency = "currency";
	private static final String CURRENCY = "currency";

- * 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.
+ */

feat: add shop-screen #22

feat: add shop-screen #22

Conversation

ahv15 commented Jul 15, 2021 • edited by jdrueckert Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Aug 24, 2021

ahv15 commented Aug 24, 2021

jdrueckert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahv15 Aug 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syntaxi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahv15 commented Jul 15, 2021 •

edited by jdrueckert

Loading

ahv15 Aug 27, 2021 •

edited

Loading