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

feat(api): virtual components #842

Open
wants to merge 12 commits into
base: main/4
Choose a base branch
from

Conversation

kashike
Copy link
Member

@kashike kashike commented Dec 1, 2022

  • look over component flattening with virtual components
  • figure out how to do a builder that's useful for outside people

@kashike kashike changed the title feat(api): virtual component value holders feat(api): virtual components Dec 1, 2022
@kashike kashike force-pushed the feature/virtual-component-holders branch 2 times, most recently from 259561c to b3c5c43 Compare December 3, 2022 22:20
@zml2008 zml2008 added this to the 4.13.0 milestone Dec 16, 2022
Copy link
Member

@kezz kezz left a comment

Choose a reason for hiding this comment

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

overall lgtm!

do we need to make any changes to serializers to make sure they throw errors if any VirtualComponent instances get through unrendered?

additionally this likely needs MM changes too to add this component type (unless we're happy with it just throwing errors?)

@zml2008
Copy link
Member

zml2008 commented Feb 12, 2023

@kezz right now the virtual component is treated as/turns into a text component whenever its serialized -- when we get a public custom serializer API on MM we will have to make sure user-provided serializers can claim virtual components before the built-in serializers get a say.

@kashike kashike force-pushed the feature/virtual-component-holders branch from 702cbeb to b6052d1 Compare February 13, 2023 04:06
@zml2008
Copy link
Member

zml2008 commented Mar 14, 2023

After fiddling around with this a bit to use the API in MM, I think this will need some more iteration before it's ready to push out.

Some key things:

  • equality and examinableness of virtual holders
  • ensuring api operations preserve virtual components where possible
  • thinking about the level of boiler plate needed to create data -- do we need the two-layer system?

@zml2008 zml2008 removed this from the 4.13.0 milestone Mar 14, 2023
@zml2008 zml2008 dismissed their stale review March 14, 2023 05:27

changes made

@Pablete1234
Copy link

thinking about the level of boiler plate needed to create data -- do we need the two-layer system?

Would it make sense to, instead of having the 2-layer VirtualComponent -> VirtualComponentHolder<?> -> boxed data, as well as the need for a custom renderer, have the component itself have a "virtual component renderer" instead?

A way to do that would be to make it so VirtualComponent internally has a Function<C, Component> (or functionally equivalent VirtualComponentRenderer) which is used at render time to get the actual component, replacing the current unbox. Such function is part of 3rd party plugins and so they can have as much state or non-state as they want within it, and adventure doesn't really need to care about it. This essentially shifts the focus of the "virtual component holder" so instead of "holding data" it's the "how does this get rendered"

An example of a simple component which would show each player their own name:
virtual(context -> text(context.getOrDefault(Identity.NAME, "no name :(")));

Main issue I see (correct me if i'm wrong) is context is generic in adventure, and only in platform does it get a specific type, so one platform may use pointered and others may use something else? i have not looked into that one.

equality and examinableness of virtual holders

I think that just goes out the window, simply because it no longer becomes adventure's territory, it's a "whatever that 3rd party plugin decided" territory. How relevant is it to adventure how internally the renderer of the 3rd party plugin decides to be? Do equal-ness checks break adventure in significant ways if they're not respected for the renderer of virtual components?

@kashike kashike force-pushed the feature/virtual-component-holders branch from 2786555 to 23fcd20 Compare April 3, 2023 23:37
@zml2008 zml2008 added this to the 4.14.0 milestone Apr 25, 2023
@zml2008 zml2008 force-pushed the feature/virtual-component-holders branch from 23fcd20 to ec35df3 Compare May 15, 2023 01:44
@kashike kashike force-pushed the feature/virtual-component-holders branch from ec35df3 to d78274a Compare May 15, 2023 20:23
@zml2008 zml2008 removed this from the 4.14.0 milestone Jun 5, 2023
@Pablete1234
Copy link

Hey, any updates on this? It'd be great to see the functionality finally landing on adventure

@FxMorin
Copy link
Contributor

FxMorin commented Nov 23, 2023

I would like it if you could add a builder wrapper for use with the virtual component system.
In our project we create and use custom components. Having the ability to build on top of the virtual component system would be very helpful and would help reduce the current amount of hackery we do.

Currently the abstract builders are private (for good reasons). However we are willing to deal with breaking changes when needed, the virtual components however would solve this ongoing issue and prevent any more breaking changes from affecting us.

@zml2008 zml2008 force-pushed the feature/virtual-component-holders branch from f656eb5 to 336905a Compare February 16, 2024 14:57
@zml2008 zml2008 added this to the 4.18.0 milestone Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants