Skip to content

Add InventoryView#sendTitleUpdate(Component)#7979

Closed
nopjar wants to merge 1 commit into
PaperMC:masterfrom
nopjar:feat/update-inventory-title
Closed

Add InventoryView#sendTitleUpdate(Component)#7979
nopjar wants to merge 1 commit into
PaperMC:masterfrom
nopjar:feat/update-inventory-title

Conversation

@nopjar
Copy link
Copy Markdown
Contributor

@nopjar nopjar commented Jun 14, 2022

Adds a possibility to send a title update (packet). This does not
change the title on the server.

resolves #7950

Comment thread patches/server/0910-Add-sendTitleUpdate-for-inventories.patch Outdated
Comment thread patches/server/0910-Add-sendTitleUpdate-for-inventories.patch Outdated
Comment thread patches/api/0385-Add-sendTitleUpdate-for-inventories.patch Outdated
@nopjar nopjar force-pushed the feat/update-inventory-title branch 2 times, most recently from 6954abb to 71b8117 Compare June 14, 2022 11:24
Copy link
Copy Markdown
Member

@NoahvdAa NoahvdAa left a comment

Choose a reason for hiding this comment

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

Inline the imports (makes for a smaller diff, and less conflict on updates)

Adds a possibility to send a title update (packet). This does not
change the title on the server.
@nopjar nopjar force-pushed the feat/update-inventory-title branch from 71b8117 to 883b76c Compare June 14, 2022 12:05
@nopjar
Copy link
Copy Markdown
Contributor Author

nopjar commented Jun 14, 2022

Inline the imports (makes for a smaller diff, and less conflict on updates)

Done. Makes some lines extremely long, but nevermind I guess.

@Astralchroma
Copy link
Copy Markdown
Contributor

This would be great for some stuff I am working on, is there any chance this could be merged soon?

@lynxplay
Copy link
Copy Markdown
Contributor

I am unsure if this shouldn't live in UnsafeValues.
The behaviour is definitely not 100% intented.

@broken1arrow
Copy link
Copy Markdown

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

@Astralchroma
Copy link
Copy Markdown
Contributor

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

The related issue (#7950) listed some code which should work, so I have converted it to 1.19.2 mojang mappings for our own use.

@broken1arrow
Copy link
Copy Markdown

I am unsure if this shouldn't live in UnsafeValues. The behaviour is definitely not 100% intented.

Yeah is not "intended" behavior but it not exist other options to update inventory tittle while is open and under my testing is it safe run it asynchronous.

Could be something even Mojang self could implement long ago, is several cool things you can do with dynamic tittle (like tell amount of x item if the inventory currently add items or get removed and so much more).

@broken1arrow
Copy link
Copy Markdown

broken1arrow commented Aug 14, 2022

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

The related issue (#7950) listed some code which should work, so I have converted it to 1.19.2 mojang mappings for our own use.

Yeah that are my older code. I made a better upgraded version with less repeated code (if I'm not update that).

Or do you use the Mojang mappings directly?

@Astralchroma
Copy link
Copy Markdown
Contributor

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

The related issue (#7950) listed some code which should work, so I have converted it to 1.19.2 mojang mappings for our own use.

Yeah that are my older code. I made a better upgraded version with less repeated code.

Interesting, could you send that "upgraded version"?

@lynxplay
Copy link
Copy Markdown
Contributor

Yeah is not "intended" behavior but it not exist other options to update inventory tittle while is open and under my testing is it safe run it asynchronous.

I am not saying that it isn't working, it certainly works right now.
I am arguing that it may be better in unsafe values as this functionality may stop working at any point in the future as it is not intended behaviour.

@Astralchroma
Copy link
Copy Markdown
Contributor

Yeah is not "intended" behavior but it not exist other options to update inventory tittle while is open and under my testing is it safe run it asynchronous.

I am not saying that it isn't working, it certainly works right now. I am arguing that it may be better in unsafe values as this functionality may stop working at any point in the future as it is not intended behaviour.

That or maybe a sort of unsafe API annotation, along with clear documentation that it is unsafe.

@broken1arrow
Copy link
Copy Markdown

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

The related issue (#7950) listed some code which should work, so I have converted it to 1.19.2 mojang mappings for our own use.

Yeah that are my older code. I made a better upgraded version with less repeated code.

Interesting, could you send that "upgraded version"?

was on mobile and to lazy to check, but that are pretty updated, but not hurt if made more improvements like not have several loadNmsClasses() methods some do same thing more or less. But yeah as always time is your enemy :)

@broken1arrow
Copy link
Copy Markdown

Yeah is not "intended" behavior but it not exist other options to update inventory tittle while is open and under my testing is it safe run it asynchronous.

I am not saying that it isn't working, it certainly works right now. I am arguing that it may be better in unsafe values as this functionality may stop working at any point in the future as it is not intended behaviour.

In theory should it work on newer versions (so long mojang not remove the packet some get send to player or get moved).

If i understand how paper compile the jar (they add the different nms mappings already (so this will find right classes) and so long mojang not remove that code to send packets I think it will exist for long time).

@Astralchroma
Copy link
Copy Markdown
Contributor

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

The related issue (#7950) listed some code which should work, so I have converted it to 1.19.2 mojang mappings for our own use.

Yeah that are my older code. I made a better upgraded version with less repeated code.

Interesting, could you send that "upgraded version"?

was on mobile and to lazy to check, but that are pretty updated, but not hurt if made more improvements like not have several loadNmsClasses() methods some do same thing more or less. But yeah as always time is your enemy :)

Ah, well I only really need 1.19.2 as the plugin is designed for a specific server, so I've just done this.

// NMS trickery to update the Inventory name without re-opening it
// See https://github.com/PaperMC/Paper/issues/7950 and https://github.com/PaperMC/Paper/pull/7979
(event.whoClicked as CraftPlayer).handle.connection.send(
	ClientboundOpenScreenPacket(
		(event.whoClicked as CraftPlayer).handle.containerMenu.containerId,
		(event.whoClicked as CraftPlayer).handle.containerMenu.type,
		PaperAdventure.asVanilla(buildPageText(target, pageNumber))
	)
)

@broken1arrow
Copy link
Copy Markdown

This would be great for some stuff I am working on, is there any chance this could be merged soon?

If this not will be added, I can give code some work from 1.8.8 to at least to 1.19.1. but newer versions it use json to string

The related issue (#7950) listed some code which should work, so I have converted it to 1.19.2 mojang mappings for our own use.

Yeah that are my older code. I made a better upgraded version with less repeated code.

Interesting, could you send that "upgraded version"?

was on mobile and to lazy to check, but that are pretty updated, but not hurt if made more improvements like not have several loadNmsClasses() methods some do same thing more or less. But yeah as always time is your enemy :)

Ah, well I only really need 1.19.2 as the plugin is designed for a specific server, so I've just done this.

// NMS trickery to update the Inventory name without re-opening it
// See https://github.com/PaperMC/Paper/issues/7950 and https://github.com/PaperMC/Paper/pull/7979
(event.whoClicked as CraftPlayer).handle.connection.send(
	ClientboundOpenScreenPacket(
		(event.whoClicked as CraftPlayer).handle.containerMenu.containerId,
		(event.whoClicked as CraftPlayer).handle.containerMenu.type,
		PaperAdventure.asVanilla(buildPageText(target, pageNumber))
	)
)

Yeah that are better way :) sadly I need it to work on any minecraft version.

@lynxplay
Copy link
Copy Markdown
Contributor

In theory should it work on newer versions (so long mojang not remove the packet some get send to player or get moved).

Yes the method will still be able to "rename" an inventory, however the fact that there is any difference between the method and simply opening a new inventory with the same item is completely up to how the client handles a reused container id.

E.g. the client may simply properly open a new (or well the same) inventory when said packet it send in the future, resetting the mouse on the screen similar to opening a new inventory.
This is what I am referring to when I am saying that this method may be unstable/abuses not intended behaviour.
As there is not specific packet for renaming an open window and vanilla never uses this logic, this method relies on the specific quirk in the client to deliver different functionality from just opening a new inventory with the same items.

@broken1arrow
Copy link
Copy Markdown

In theory should it work on newer versions (so long mojang not remove the packet some get send to player or get moved).

Yes the method will still be able to "rename" an inventory, however the fact that there is any difference between the method and simply opening a new inventory with the same item is completely up to how the client handles a reused container id.

E.g. the client may simply properly open a new (or well the same) inventory when said packet it send in the future, resetting the mouse on the screen similar to opening a new inventory. This is what I am referring to when I am saying that this method may be unstable/abuses not intended behaviour. As there is not specific packet for renaming an open window and vanilla never uses this logic, this method relies on the specific quirk in the client to deliver different functionality from just opening a new inventory with the same items.

Sry for late replay. It will not reset the mouse this method if is what you referee to, open same menu again not break anything. The only way to break it, is if you send wrong packet to the client (hopper inventory instead of chest or wrong size). But you can easy build away this issues with player method (get the inventory player open that are 99% fail safe then).

What this exploit or use is the packet some server send to player (i don't think mojang can or will remove that because even mojang self really on that packet).

@Owen1212055
Copy link
Copy Markdown
Member

Owen1212055 commented Nov 26, 2022

Although this is rather nice, it's a bit hacky as we need to resend the entire inventory like this.
What are some of the example use cases for this? Because at least, I'm not sure this is something we should encourage people to use.

@broken1arrow
Copy link
Copy Markdown

broken1arrow commented Nov 26, 2022

Although this is rather nice, it's a bit hacky as we need to resend the entire inventory like this. What are some of the example use cases for this? Because at least, I'm not sure this is something we should encourage people to use.

Here is examples (but you can do so much more, like animate the title)
https://youtu.be/FvGoCpEh5cY

use the close inventory trick, break it for you (you can't have items on the cursor then). This packet is send async or it will be buggy.

Right now i has to hock in to nms and need check every minecraft version, so i know it still works. It could be better ways, but is mojang self use this but not in same way as i do it.

@Owen1212055
Copy link
Copy Markdown
Member

I see, so you're using it to show information. That's interesting, but, maybe i'd mark this api as @ApiStatus.Experiemental?

@broken1arrow
Copy link
Copy Markdown

broken1arrow commented Nov 26, 2022

I see, so you're using it to show information. That's interesting, but, maybe i'd mark this api as @ApiStatus.Experiemental?

You can see my mess to class here #7950 (I think I has done some improvements some are not updated in the tread). But should work from 1.8.8 to 1.19.2 :)

Yes or page numbers, but you can do more like animations and in 1.16+ you can also exploit gradients to make crazy animated patterns (I not try it because I know that can be laggy).

@broken1arrow
Copy link
Copy Markdown

broken1arrow commented Nov 26, 2022

Ether way can be smart add @ApiStatus.Experiemental :) I mean if you not use it correctly, you will get strange issues (I don't know exactly the implementation plan in paper). Is not good idea send wrong inventory size in the packet to player (that not end up right at all) and player can end up lose items (no mater if it a "menu" or "custom inventory you store items").

@Owen1212055
Copy link
Copy Markdown
Member

Thankfully we shouldn't have to worry about the wrong inventory sizes being sent, as the bukkit impl should have those values already given.

@broken1arrow
Copy link
Copy Markdown

Thankfully we shouldn't have to worry about the wrong inventory sizes being sent, as the bukkit impl should have those values already given.

yeah but the methods i use I need also specify right inventory and size or it will end up wrong inventory for client :) no mater the inventory player accentually open (but has not full view how it will be implemented in paper).

Ether way should be safe use it async (only side-effects in some rare cases it not update to latest info (but update it one more time and it is fine)).

@nopjar
Copy link
Copy Markdown
Contributor Author

nopjar commented Apr 7, 2023

It seems like that this change is not really a thing. That's why I am closing it.

@nopjar nopjar closed this Apr 7, 2023
@broken1arrow
Copy link
Copy Markdown

broken1arrow commented Apr 7, 2023

It seems like that this change is not really a thing. That's why I am closing it.

It is ofc a thing and give me as developer more control over the title. No current method allow to change title without open inventory again.

I have use it from 1.8.8 to 1.19.4 and one plugin use my more hacked method on 100+ servers and 1000+ users has seen this without realizing the title update on the fly.

Add option directly, give me more time do other stuff than keep update for every Minecraft version.

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.

Add method to update title when inventory is open

8 participants