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

Expand the CommandSourceStack interface by exposing mutators #11866

Open
emilyy-dev opened this issue Dec 29, 2024 · 8 comments · May be fixed by #11868
Open

Expand the CommandSourceStack interface by exposing mutators #11866

emilyy-dev opened this issue Dec 29, 2024 · 8 comments · May be fixed by #11868
Labels
status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added.

Comments

@emilyy-dev
Copy link
Member

Is your feature request related to a problem?

While brigadier is capable of redirecting or forking a node modifying the command source via (Single)RedirectModifier, it is impossible to make use of this feature using the current command API

Describe the solution you'd like.

Expand the CommandSourceStack interface by adding wither methods:

  • CommandSourceStack withExecutor(Entity executingEntity)
  • CommandSourceStack withLocation(Location location) (atLocation?)
  • For the sake of completion and because Minecraft's CSS also has this method, CommandSourceStack withSender(CommandSender sender), but I'm not really sure how to feel about this, it's one to discuss: do we want to retain the original sender or be able to change it? We can also do both by exposing an additional originalSender method as well, or simply not add such method today but can be revisited in the future

This would allow for plugins to use the forwarding mechanisms of brigadier and better integrate with it without needing to give players permission to run /execute or API users recoursing to use internals

Describe alternatives you've considered.

Using internals or giving players permission to use /execute

Other

No response

@NonSwag
Copy link
Contributor

NonSwag commented Dec 29, 2024

I am not really familiar with the internals here, but maybe it would be possible to add a builder method or something to be able to create a copy while keeping the original object unmodified

@emilyy-dev
Copy link
Member Author

emilyy-dev commented Dec 29, 2024

They would return a new CSS instance, that's how Minecraft's CSS works

@masmc05
Copy link
Contributor

masmc05 commented Dec 29, 2024

Maybe it would also be possible for plugins to store their own data in CSS? Same way vanilla stores there the executor and location, which it can change. CSS is a short living object that dies anyway when command finished executing, so don't think it'll be a big issue if plugin objects would be stored in that nms class

@masmc05
Copy link
Contributor

masmc05 commented Dec 29, 2024

For the sake of completion and because Minecraft's CSS also has this method, CommandSourceStack withSender(CommandSender sender

This one sounds like it'll end up misused a lot, and without many real usages other than a plugin (either the same one or other plugins) not working great with withExecutor

@pop4959
Copy link
Member

pop4959 commented Dec 29, 2024

Maybe it would also be possible for plugins to store their own data in CSS? Same way vanilla stores there the executor and location, which it can change. CSS is a short living object that dies anyway when command finished executing, so don't think it'll be a big issue if plugin objects would be stored in that nms class

There are already numerous utilities for storing data and for a short lived object like this it probably isn't necessary at all anyways.

@Machine-Maker
Copy link
Member

nms's CommandSourceStack#withSource isn't used except for debugging, so no, I would 100% be against adding that.

@emilyy-dev
Copy link
Member Author

nms's CommandSourceStack#withSource isn't used except for debugging, so no, I would 100% be against adding that.

fair enough, didn't know that, so that one's out of the table then

@masmc05
Copy link
Contributor

masmc05 commented Dec 29, 2024

There are already numerous utilities for storing data and for a short lived object like this it probably isn't necessary at all anyways.

There is no reliable way. Ofc we can use something like WeakHashMap but then if something uses the new with* methods and creates a new one and propagate further that one, your data is lost. A lot easier it would be to store the data in the object itself which would be propagated to the execution and then would die easily

@Machine-Maker Machine-Maker added status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added. and removed status: needs triage labels Dec 29, 2024
@Strokkur424 Strokkur424 linked a pull request Dec 30, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepted Disputed bug is accepted as valid or Feature accepted as desired to be added.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants