[1.14.4] I am doing a lot of things in the .render() of the GUI. Need help with finding the right place to handle those things.

[Cerandior]

I am making a villager-type entity who sells the player cool tools in exchange for gold.

I got pretty much the whole thing handled right now, however I am doing a lot of the things in the .render() method of the GUI Screen that I feel don't belong there. I didn't know where else to put them so I just stuck with it, but I don't think it's ideal.

I am guessing the render() method is called once per frame and that's why it was helpful to check for stuff the player is doing in the GUI, but it probably isn't the best idea to do other stuff besides rendering there. Can anyone help me "relocate" some of the stuff I am doing at the render() method right now?

So what am I doing currently that I think doesn't belong there:
1) Handling the visibility of the buttons of the trades based on the number of trades the entity has.
2) Checking which trade the player has selected based on an index which takes a value from a button click and based on that selected trade I change the contents of a slot of the container.
3) Checking whether the player has input the right amount of gold in one of the container slots to display the requested trade item in another slot.
4) Checking when he takes this gold out in order to take the trade item out as well so the player doesn't take stuff for free.
5) Checking if there is no quantity available for a given trade to disable the button responsible for selecting that trade.

 

In case I missed anything you could also look at the Screen class here:
https://github.com/arjolpanci/VanillaExtended/blob/master/src/main/java/teabx/vanillaextended/client/gui/WanderingAssassinScreen.java

Thank you for your help.

[Cerandior]

34 minutes ago, diesieben07 said:

Why is this client side at all? Slot modification should be server side...?

 

Again, why is this on the client at all?

 

This especially needs to be on the server, since it prevents item-duping. You cannot trust the client. At all. Ever.

Consider making a custom Slot class to handle the appropriate event by overriding a method (e.g. onTake).

 

Firstly. Thank you for telling me about the tick method. Didn't know about it.

Now about the Slot handling which is actually the most important thing. I was experimenting with stuff to see how could I achieve the trade interaction and when I changed slot contents from the GUI (clientside) a bug which actually turned out to be a desirable effect occurred.

Basically because I am adding items to an inventory from the client, the server doesn't know about that item, so when you try to pick it up, you can't. And because I was adding the item from the render() method, that would replace the item every frame if the player tried to take it.

So I used that to make a "display" Slot, which never gets updated to the server. Its purpose is only for the player to check out the item he is about to purchase. Then when the player is actually paying enough gold for the item, I send a packet to the server to update that Slot content so the player can take it.

 

It seems like I shouldn't be doing this sort of thing though, so maybe I have to find another way to make that display Slot work the same way. Also, when you say that I should be handling slot modification server side, how exactly am I supposed to do that when I am working with the input of the player within a GUI, which is only run on the client.

Thank you for your time.

 

[Cerandior]

5 minutes ago, diesieben07 said:

The input needs to be sent to the server (in a "this is what the player did" fashion, i.e. "button X was clicked with 123 in text field A" not "update value foo to 768"). The server then needs to validate that these actions are proper (the player actually has the container open and is still in range [which is checked by Container#canInteractWIth] and the action is valid (e.g. numbers are in the allowed range, etc.)). Again remember. The client cannot be trusted. If you trust it, people will cheat.

Then the server performs the necessary actions that result from the player's input (e.g. moving a stack around in the slots or producing a resulting item and taking the input). If necessary it then sends data to the client to update its state (in the case of GUI slots that happens automatically by vanilla).

As far as I am understanding, I am still going to work with the input of the GUI, but I must send detailed packets to the server for each action of the player to check whether that action is legit or not?

[Cerandior]

Okay, once again, thank you for your help.

I reworked everything and I would really appreciate it if someone can have another look and tell me if there is still something wrong with the way I am handling things.

All my packets can be found here:
https://github.com/arjolpanci/VanillaExtended/tree/master/src/main/java/teabx/vanillaextended/network

And my packets are used by:
My GUI Buttons
My Custom Slot
My Entity

Also, when I am sending packets to the Client, I am sending it to all clients at the moment. That is because I am not familiar with the java Supplier and I am not quite sure what I am supposed to pass as an argument to PacketDistributor.PLAYER.with() since it asks for a Supplier.

[Cerandior]

2 hours ago, diesieben07 said:

• Why is your screen constructor sending a packet? The server knows when the container is opened (the server is the one opening it!). It can just perform this action immediately.
• When opening the GUI you are sending a packet to all players. Why?

1) That is actually quite stupid now that I am looking at it again. I was doing this very late in the night yesterday. The screen was saving the previous selection and when I reopened the GUI I wanted to reset. And since I was working with packets for a while, the first solution that I thought of was sending a packet. Absolute genius.

2) Well, if I don't send a packet to the client, the offerlist of the container is empty for some reason. Or perhaps you are not questioning why I am sending the packet rather why I am sending it to all players. If that is the case, this is the reason:

15 hours ago, Cerandior said:

Also, when I am sending packets to the Client, I am sending it to all clients at the moment. That is because I am not familiar with the java Supplier and I am not quite sure what I am supposed to pass as an argument to PacketDistributor.PLAYER.with() since it asks for a Supplier.

 

2 hours ago, diesieben07 said:

• Why are your Slot methods sending stuff to the server? The slot methods are already called on the server. Inventory management inside a container is already implemented in vanilla networking code.
• In general your packet names suggest you are thinking about this in the wrong way still. The client is telling the server what to do (e.g. "ValidateTradeInput"). The client should be informing the server of what the player did.

This is the problem. You are right, I am not sure how to handle this interaction. The slots were sending stuff to the server, because I can get the Player who send it by doing that, and from the player I can get the container and so on. I am kind of doing the same thing as before, but now I am checking for stuff in the packet instead of before notifying the server. Which maybe is kind of the same thing since I am still telling the server to update slot contents based on what was changed in the client.

I am not sure what I am supposed to do though. The only input I have from the player is two slots and some buttons. The buttons are only used to change the selected trade, so not much to handle there, so all I am left with are the slots. What should I be telling the server except that the player changed the contents of some slot and took some item from another. There is no other information that I can think of that should be sent to the server.

So with the onSlotChanged() I notified the server that the contents of the slot were changed, so then I check in the packet if the right container is opened by the sender. If he actually can use that container, and if the new contents of the slot he changed are in range of some specific values. If yes, I put the selected trade item in the other slot so he can take it.
Then with the onTake() I check again for the previous conditions and decrease the stack size of the gold based on the price of the item.

That's what I had in mind when doing all of this. Which part of that chain is wrong? (Assuming I am not entirely wrong)

Edited March 22, 2020 by Cerandior

[Cerandior]

17 minutes ago, diesieben07 said:

Vanilla already does all this network stuff for you. The Slot methods are already called on the server without any further checks from you required. You don't need to send a packet, just check for !World#isRemote and do the stuff on the server directly.

 

So do I just pass in the container and the player to the Slot and do the checking there?

[Cerandior]

1 hour ago, diesieben07 said:

Yes. Either that or call methods on the Container. I'd prefer to keep all the logic in one place (the container). But thats a stylistic choice.

Okay, I re-did the whole thing again.
It's the same stuff I was doing before but everything it's on the slots and the container now.
Also I kept the packets for the buttons, because if I don't do that the player would have to update the contents of the gold slot when he changes to another trade to get the new trade item. I want it to check for the input when the trade is changed as well so the new Item can be displayed immediately.

 

Container
Slot
Screen
Entity

 

Thank you for your help and your patience.

Edited March 22, 2020 by Cerandior

[Cerandior]

20 minutes ago, diesieben07 said:

Still don't know why you are sending two packets from the one button. You should send one packet saying "user pressed the button" and then let the server decide what to do with that. Sending two packets allows room for clients to be naughty (by sending only one packet for example).

Yes, I was working with that right now after I posted this. Just said that I am keeping the packet for the button for the reason I mentioned. Is everything else fine now?

Edited March 22, 2020 by Cerandior

[Cerandior]

1 minute ago, diesieben07 said:

The rest looks fine, yeah. Just that (unrelated to this topic) you are still using java serialization.

Oof, yeah it seems I forgot to change that for the entity. For the packets I switched to using for loops. It's kind of the same thing happening in the packet too, so I am just copying it from there.

Thank you for everything.