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

Inventory and generic interactions #1252

Merged
merged 26 commits into from
Oct 16, 2014

Conversation

flo
Copy link
Contributor

@flo flo commented Oct 3, 2014

This patch series makes it very easy to create new dialog based interactions with blocks. It also converts the existing chest access to the new system.

All interactions based on the new system will have the following features:

  • If the interaction target has an inventory, shift click can be used to transfer items.
  • When you move out of interaction range, the interaction stops and the open interaction screen closes
  • The interaction hotkey E can be used to stop interacting. This also closes a dialog if there is one.
  • Opening the inventory with hotkey I, will also abort the interaction.
  • Interaction screens open immidatly even if there is network delay. The server validates afterwards if it was okay. In rare cases where that isn't the case the screen will close again.

Modules that added new chests that use the old mechanism need to perform the following adjustment to their entities:

-  "AccessInventoryAction": {},
+  "InteractionTarget": {},
+  "InteractionScreen": {
+    "screen": "engine:containerScreen"
+  },

The InteractionTarget component triggers the sending if interaction events and the setting of the interaction fields.The InteractionScreen behavior will show the specified dialog when the user starts interacting with it. Without any further system implementation the interaction features listed above will apply: So it's possible to add new dialogs without implementing any event processing system!

It should also be possible to extends the workstation module so that it makes use of the new interaction system. That way all workstation based modules would get interaction powered dialogs.

For more details please have a look at the commit messages, which often have more than one line.

Noteworthy other changes:

  • The sound of actions is now also played without network delay
  • Clients can see the damage done to doors.
  • API wise it's now possible to react to closed screens via a new event.

@flo flo force-pushed the inventory-and-generic-interactions branch from f893c0e to 2530aed Compare October 3, 2014 12:35
public float interactionRange = 5f;
/**
* Specifies the current interaction target. It points for example to a standard chest when the player has opened it.
Copy link
Contributor

Choose a reason for hiding this comment

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

If a setter should be used, wouldn't it be better to make this a getter/setter pair backed by a private field? Or does that interfere with serialization?

@flo
Copy link
Contributor Author

flo commented Oct 3, 2014

The interaction target could move away too. E.g. when the chest is in a minecart. So if I don't do it in a loop I would have to do it at character and target movement.

Since interactions are limited to characters there won't be that many interactions going on at the same time. So performance should not be an issue.

So I went for this solution for simplicity.

About the interactionTarget field being public: No component field I have seen so far uses a getter and setter. So I assume there is a reason for this practice.

@Cervator
Copy link
Member

Cervator commented Oct 3, 2014

Cool stuff :-)

This might also be of interest to @MarcinSc who did some inventory work in the past, and of course @immortius at an architectural level.

I believe the public field is a thing, yeah.

@Cervator
Copy link
Member

Cervator commented Oct 7, 2014

Bump - @immortius or @MarcinSc either of you out there with any thoughts on this?

@flo - before merging this would you entertain an additional request from me? :-)

This PR has a special request to properly use the new feature that'll get lost the moment this PR is closed, along with several other cool features added elsewhere over time but then not left obvious enough for anybody to find and use.

We don't really have any serious docs or instructions on how to use inventory-bearing entities. Would you be up for expanding the javadoc and/or (preferably and!) enhancing the wiki page a bit? https://github.com/MovingBlocks/Terasology/wiki/Inventory

There's also a separate page for a Chest - https://github.com/MovingBlocks/Terasology/wiki/Chest - but I think that should be deleted as too specific for the wiki. Long-term my design hope is to run one forum thread per major feature like this in either Core Projects (http://forum.terasology.org/forum/core-projects.54 - feel free to post an Inventory thread for doc/functionality discussion) or the Module forum and match that with a wiki page (or for modules a readme or whole wiki depending on its depth)

@flo
Copy link
Contributor Author

flo commented Oct 7, 2014

I created a forum post. I might change the concept / pull request a bit based on the feedback. For a wiki entry it's to early I think. I don't even know if it will make it in.

@Cervator
Copy link
Member

Cervator commented Oct 8, 2014

flo added 22 commits October 9, 2014 13:01
The current interaction target of a character (e.g. a container) gets now
stored within a new CharacterComponent field called interactionTarget.

A new utility class CharacterUtil can be used to easly specify the current
interaction target of a character.

At the start of an interaction an InteractionStartEvent gets sent to
the target entity (e.g. the container).
Systems can listen to this event to open certain UI dialogs.
As the event is sent to the target, it is possible to filter for
interaction events of entities with certain components.
This event replaces the old OpenInventoryEvent.

At the end of an interaction an InteractionEndEvent gets sent.
Systems can listen to this event to close certain UI dialogs.
Reasoning:
1. The field does not get used
2. The field didn't contain usefull values:
    -> The list got only extended but never reduced.
3. Via the interactionTarget field of the character,
    it is now also possible to check if a player is allowed to perform
    an access.
Reasoning: Preperation to make the container screen configurable via a new InventoryUIComponent.
The new InteractionScreen component can be added to entities to make them
open a custom dialog during interactions triggered by hotkey E.

This new concept gets now also used for chests.
…sed.

A module developer that usess the InteractionScreen component should not
need to know about the need to update the interactionTarget field
of the character.

This patch makes will make automatically opened interaction screens,
also automatically update the interactionTarget field when they get
closed.

For that purpose a new event is introduced called ScreenLayerClosedEvent
which gets sent to the client component when a screen layer gets closed.
Reasoning: One of it's method has little to do with a charcter.
…eived.

This commit contains also some minor other changes like
javadoc and making InteractionEndEvent go to the owner only.
This change adds a ActivationRequestDenied event.
The ActivationRequestDenied event is sent when the server determined
that the activation done by the client is invalid.

The ACtivationREquestDenied event is then used to trigger the
interaction end, which will result in a closed interaction window.
@flo flo force-pushed the inventory-and-generic-interactions branch from 2530aed to 99c4922 Compare October 12, 2014 21:17
@flo
Copy link
Contributor Author

flo commented Oct 12, 2014

There seems always more to fix and improve but I think I should stop at this point and wait till this pull request gets through. Afterwards I plan to prepare some pull requests for some modules that used the old AccessInventoryAction component. Tomorrow I plan to update the forum post to explain the events in this new pull request further.

The main new thing is, that the interaction get now predicted on the client in a predictedInteractionTarget field. The authority validates the interaction and stores the current interaction in authorizedInteractionTarget.

@Cervator
Copy link
Member

Okay so we put this PR back into the review cycle for merging soon'ish then :-)

@Cervator
Copy link
Member

Tested this locally in my integration workspace with all stable modules. We need to fix the following modules to go along with this change:

  • Journal - @MarcinSc but he might be in the middle of moving across an ocean
  • Rails - @small-jeeper - heads up!

@flo - could you review real quick and recommend what might be needed? If easy maybe we can throw matching PRs together and I can merge those at the same time :-)

@Josharias - I guess Machines/Workstation do not fail but they work better? Cool! (re: forum)

@Cervator Cervator merged commit 99c4922 into MovingBlocks:develop Oct 16, 2014
@Cervator
Copy link
Member

Okay, not sure what was wrong with my workspace last night, but I was clearly too sleepy to figure it out :-)

Journal is fine as is some other stuff that looked weird. Rails is still broken, but I'm sure @small-jeeper can figure it out, will poke him separately and reference the forum thread

Thanks @flo !

@flo
Copy link
Contributor Author

flo commented Oct 16, 2014

Thanks for merging @Cervator !

I checked the Rails module, but apparently the compilation issue got already fixed by @small-jeeper
Thanks for the quick rails fix @small-jeeper, when you have some questions regarding the new functionality please let me know.

All modules which uses the AccessInventoryAction component, need the following adjustment:

-  "AccessInventoryAction": {},
+  "InteractionTarget": {},
+  "InteractionScreen": {
+    "screen": "engine:containerScreen"
+  },

Otherwise the custom chests they added can't be used. I will prepare some pull requests.

@flo
Copy link
Contributor Author

flo commented Oct 16, 2014

Wow apparently @Josharias did already update the WoodAndStone, Workstation and TerraTech
module to use the new functionallity. And he did not just the small change described above but actually switched the Workstation screens to the new system too.

I created a pull request for CopperAndBronze: Terasology-Archived/CopperAndBronze#2

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.

3 participants