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

Prototype configuration tool #1663

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

warjort
Copy link
Contributor

@warjort warjort commented Jun 7, 2021

What:
This is a prototype configuration tool for #1588

How solved:
Added 2 new capabilities
IConfigurable implemented by machines and covers that gets and sets a subset of the NBT that is just the configuration.
IConfiguratorItem a tool that can retrieve this config from one machine/cover and apply it to another

Outcome:
WIP

Additional info:
This has been implemented as "opt in", each individual machine and cover has to expose the IConfigurable capability.
This is because each machine/cover needs to decide what is configuration and what is other state.

For now the SimpleMachine and Conveyor cover (including item filters) have been implemented.
NOTE: item filters as covers themselves have not been implemented (though it would be easy to do).
It is only a subset so people can try it without me having to unpick everything if people don't like it.

The configurator tool is actually the FILE (this is because it is only a prototype and the file has no other real use).
It has 2 modes, you change it (like similar tools in other mods) by pointing away from blocks and right clicking.

COVER - this is the normal mode that respects the machine grid and lets you configure covers
MACHINE - this ignores the grid and lets you see through covers - it is mainly for people who are having difficulty finding somewhere to click to change their machine

For the use of the tool itself, shift right click is to save the configuration and normal right click applies it.

Some issues:

  1. This is all server side. How to store something in the configurator nbt so something can be displayed in its tooltip about the configuration stored (and it would also be useful for context in chat/error messages)? You can use /ct hand to see what is stored in the item but thats not user friendly :-) UPDATE: mainly resolved except the hacky calculation of the translation key.
  2. I left some comments in the code about edge cases - e.g. what happens if a machine says it is configurable but then returns null for its configuration - for now it is just ignored with a message in chat
  3. Should the real configurator take durability - for now it does - maybe it should be an electric item?
  4. I haven't attempted to do the magic wand/auto item filter switch stuff asked for by eutro UPDATE: this has been implemented
  5. LongStringNumbers suggested (4) should be on an advanced version of the item - having a gui that lets you select from multiple saved configurations could also be an advanced feature? UPDATE: see 4
  6. The normal (de)serializeNBT is not quite the same api as this new copy/pasteConfiguration - the new api includes validation and a player so that errors/warnings can be reported - this makes reuse between the two harder

@warjort warjort marked this pull request as draft June 7, 2021 19:58
@warjort
Copy link
Contributor Author

warjort commented Jun 8, 2021

For point (1) above, I have added an IConfigurable.getConfigurationName()
This is now displayed in the tooltip of the configurator so a user can see what configuration it holds.
It is also used in some of messages sent to chat.

This implementation feels very hacky, but it does work.
The good news is that if it is wrong in some cases, individual machines or covers can change the behaviour.

@warjort
Copy link
Contributor Author

warjort commented Jun 8, 2021

I changed the IConfigurable to not use an EntityPlayer directly and instead use a context object.

This will allow additional information to be passed, e.g. whether it should use advanced tool behaviour

It also allows things other than the tool to use the api without having to do the FakePlayer hack.

@warjort
Copy link
Contributor Author

warjort commented Jun 9, 2021

I have done an implementation for points 4/5 above.

Rather than try to guess/define what advanced features should be enabled, I have done this by making the ConfigurationContext passed into the IConfigurable api an ICapabilityProvider.
This means the "caller" can decide what advanced features to enable by providing a suitable capability.

For the configurator item this has been implemented as proxying the player capabilities when it is the advanced version with the basic version having no capabilities.

Enabling this for the item is as simple as:

addComponents(new ConfiguratorItemStat().advanced());

I also made the isAdvanced() available on the ConfigurationContext and IConfiguratorItem in case somebody wants to do some advanced behaviour that does not neatly map to a capability or a capability would be overkill.

@warjort
Copy link
Contributor Author

warjort commented Jun 9, 2021

With the above changes, the "magic wand" behaviour now works for an advanced configurator.

The ItemFilterContainer will now check if there is an IItemHandler capability and use that as the inventory to check to obtain filters when the one in the cover is wrong (or place an unwanted filter into it if no filter is required in the new configuration).

I also made these features usable outside the IConfigurable api.
Although that is of limited use for the swapFilter() method since other methods to actually configure the new filter are not public.

@warjort warjort marked this pull request as ready for review June 9, 2021 14:01
@warjort
Copy link
Contributor Author

warjort commented Jun 9, 2021

I am marking this as ready for review, even though there are obviously missing pieces like;

  • the real configurator item
  • updating other machines/covers to work with the new api

These require feedback before proceeding.

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.

2 participants