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

Rapid packet-based entity data changes #2540

Merged
merged 25 commits into from Oct 26, 2023

Conversation

tal5
Copy link
Member

@tal5 tal5 commented Sep 18, 2023

Changes

  • EntityHelper#modifyInternalEntityData now takes in a MapTag, to match the new method & share it's internal handling.
  • Minor update to entity data ID mapping, display entities have a new one now.
  • The new PacketHelperImpl(1.20)#sendAsyncSafe's code was slightly changed to work properly without network interception being enabled.

Removals

  • EntityHelper#mapInternalEntityDataName was removed in favor of having that logic/handling in NMS.

Additions

  • EntityHelper#convertInternalEntityDataValues - converts a MapTag to a list of DataValue's (returned as List<Object>), used to parse entity data in the new command, implemented as such a bridge method so that it can be done outside of the async task.
  • PacketHelperImpl#sendEntityDataPacket - sends an entity data update packet to a list of players, taking in a List<Object> (DataValues).
  • fakeinternaldata command - sends fake entity data updates, optionally animating them with sub-tick precision.
  • EntityHelperImpl(1.20)#getDataItems - util method to get an entity's internal id -> DataItem map.
  • EntityHelperImpl(1.20)#convertToInternalData - internal util to parse a MapTag into entity data values, taking in a BiConsumer to use the converted values.
  • PacketHelperImpl(1.20)#sendAsyncSafe - util to send packets async, as this was done in 2 places now.
  • DenizenNetworkManagerImpl#getConnection(Player) - util to get a connection from a bukkit player.
  • Internal Entity Data language doc - a general explanation of internal entity data with relevant links, used in both the command and the mechanism.

Notes

  • Did my best to make this relatively clean, but overall it's still a pretty messy setups doing to passing NMS objects around outside of NMS and all; sadly there isn't really a way to make this nicer other than having all of the logic in NMS (or making some over the top API for ourselves), but let me know if that should be done.
  • The new command can technically be used to just send a fake entity data update to a player (which is why I've made speed optional), but is that something we want to officially document? if we do we should probably
    • Add a special case in the code to just send the update instead of doing the entire async loop thingy for a single packet.
    • What about future updates messing it up? most similar commands have packet interception to persist themselves.

@@ -85,6 +85,7 @@ public static void registerCommands() {
registerCommand(ResetCommand.class);
registerCommand(ZapCommand.class);
// entity
registerCommand(SmoothEntityDataCommand.class); // <---------
Copy link
Member

Choose a reason for hiding this comment

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

why the comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Will remove later, was a badly made TODO for myself


public FakeInternalDataCommand() {
setName("fakeinternaldata");
setSyntax("fakeinternaldata [entity:<entity>] [data:<map>|...] [for:<player>|...] (speed:<duration>)");
Copy link
Contributor

Choose a reason for hiding this comment

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

for arg should be optional and default to linked player

@tal5 tal5 marked this pull request as ready for review September 19, 2023 23:05
public FakeInternalDataCommand() {
setName("fakeinternaldata");
setSyntax("fakeinternaldata [entity:<entity>] [data:<map>|...] (for:<player>|...) (speed:<duration>)");
setRequiredArguments(3, 4);
Copy link
Member

Choose a reason for hiding this comment

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

I think 3 is wrong here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Forgot to update that, fixed

// @Usage
// Animates a rainbow glow on a display entity for all online players.
// - define color <color[red]>
// - repeat 361 from:0 as:hue:
Copy link
Member

Choose a reason for hiding this comment

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

A: hue is 0-255, not 0-360?
B: even if it wasn't a byte, the usage of 361 would still be 1 too many here (in a circle, 0 degrees and 360 degrees are the same value)

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be corrected now

// Animates a rainbow glow on a display entity for all online players.
// - define color <color[red]>
// - repeat 361 from:0 as:hue:
// - define frames:->:<map[glow_color=<[color].with_hue[<[hue]>].argb_integer>]>
Copy link
Member

Choose a reason for hiding this comment

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

constructor tag is redundant

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be corrected now

try {
for (List<Object> frame : frames) {
NMSHandler.packetHelper.sendEntityDataPacket(sendTo, entity, frame);
Thread.sleep(ms);
Copy link
Member

Choose a reason for hiding this comment

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

Should either note in meta that the delay is inaccurate for long runs, or switch this sleep to a more accurate delay manager.

Also considering the possibility of long runs, should also add a check to kill the loop if the player(s) are gone

Copy link
Member Author

Choose a reason for hiding this comment

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

Both should be fixed

// <--[command]
// @Name FakeInternalData
// @Syntax fakeinternaldata [entity:<entity>] [data:<map>|...] (for:<player>|...) (speed:<duration>)
// @Required 3
Copy link
Member

Choose a reason for hiding this comment

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

required min mismatch between meta and setSyntax

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

// Sends fake internal entity data updates, optionally sending multiple over time.
// This supports sub-tick precision, allowing smooth/high FPS animations.
//
// The input to 'data:' is a list of <@link object MapTag>s, with each map being a frame to send, with each map being formatted like <@link mechanism EntityTag.internal_data>'s input.
Copy link
Member

Choose a reason for hiding this comment

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

A command should probably be considered "more primary" than a mech (ie put the details in command and have the mech link to it) - or add a separate language defining the data that both link to

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed an initial attempt at a language doc for this, let me know if it looks good

// - fakeinternaldata entity:<[item_display]> data:[item=iron_ingot;scale=0.6,0.6,0.6]|[item=gold_ingot;scale=0.8,0.8,0.8]|[item=netherite_ingot;scale=1,1,1] speed:0.5s
//
// @Usage
// Changes an item display's item, then it's scale a second later, then it's item again another second later.
Copy link
Member

Choose a reason for hiding this comment

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

its

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

// Each entity in Minecraft has a set of data values that get sent to the client, with each data value being a number id -> value pair.
// Denizen allows direct control over that data, as it can be useful for things like setting values that would usually be blocked.
// Because this is such a direct control that's meant to impose less restrictions, there's no limitations/verification on the values being set other than basic type checking.
// For all possible internal entity data values and their respective ids, see <@link url https://github.com/DenizenScript/Denizen/blob/dev/v1_20/src/main/java/com/denizenscript/denizen/nms/v1_20/helpers/EntityDataNameMapper.java#L50>.
Copy link
Member

Choose a reason for hiding this comment

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

This doc should note somewhere that these values are liable to change between minecraft versions

@mcmonkey4eva mcmonkey4eva merged commit 0f42e74 into DenizenScript:dev Oct 26, 2023
1 check passed
@tal5 tal5 deleted the Smooth_Entity_Data_Changes branch October 30, 2023 17:27
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.

None yet

3 participants