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

Add unique identifier to each displayed image #3133

Closed
schauveau opened this issue Nov 30, 2020 · 24 comments
Closed

Add unique identifier to each displayed image #3133

schauveau opened this issue Nov 30, 2020 · 24 comments

Comments

@schauveau
Copy link

The graphic protocol provides an index for the image data (via the 'i' option) but not for the graphic element created by a 'T' or 'p' action.

So my proposal is to add an optional 'g' option to identify the graphic elements in a unique and unambiguous way. That new feature is entirely backward compatible with the current protocol.

For a 'T' or 'p' action, the graphic element with the same 'g' index, if it already exists, is first removed.

The 'd' action is extended to allow for the removal of a specific graphic element: a=d,d=g,g=VALUE

With a 'g' index, it becomes possible to implement new actions to modify the properties of an existing graphic element (size, position, image, alpha, ...).

@kovidgoyal
Copy link
Owner

Again, I need a use case. What is it you are trying to do that you
cannot with the current protocol?

@schauveau
Copy link
Author

I cannot honestly say that there is something that I cannot do with the current protocol since I can use tricks to simulate a unique id per image. The issue here is mainly about making the protocol easier to use and more reliable.

Anyways, if you really want to know my what I am up to then I want to implement an API to create graphic theme for the terminal mud client TinTin++. If you do not know MUDs, they are text games played over a telnet interface. So I do not want to create a graphic theme but a simple api that people will be able to use to create custom themes for various mud servers. The low level API will only provide a few features: create and destroy image data, draw an image or a sub-image, and remove a previously drawn image. It may also provide the ability to modify some properties of a previously drawn image (e.g. position, alpha, ...) but this is not strictly needed because that can also be obtained by removing and redrawing.

The create, destroy and draw features should be easy to implement. The problem is to remove a previously drawn image. There are currently 8 delete modes and neither of them can be used to remove a single drawing in a reliable manner.

  • 'a' removes everything
  • 'i' would only work if the image data is only used once which is not something I want to enforce.
  • 'c', 'p', 'x', 'y' rely on the screen coordinate and would fail in case of overlapping images.
  • 'q' and 'z' rely on the z-index which could also be shared by multiple images.

The trick I mentioned earlier is to abuse the z-level by insuring that each image is drawn at a different depth. For instance, if I limit the number of graphics elements to 256 (so with a g index between 0 and 255) then I can make their z-level unique using the formula z=(z<<8)+g.

So that workaround should work and, I hope, will not cause performance issues. At the end, the existence of a workaround does not explain why the protocol does not provide a simple way to reference each drawing in a simple and robust manner.

@kovidgoyal
Copy link
Owner

Do you not know where you are drawing images? Simply use the p mode for
delete.

@kovidgoyal
Copy link
Owner

And as for overlapping images, they can only overlap meaningfully if
they have different z-indices, in which case q mode does the trick.

@kovidgoyal
Copy link
Owner

If you dont know where you are drawing images, then yes, the current API is inadequate in the case that you want to draw one image at multiple places.

@schauveau
Copy link
Author

schauveau commented Dec 1, 2020

at first glance, the constraint that overlapping images should have different z-level seems reasonable but in fact this is not something that a generic API should try to enforce. Also, do not forget that images can have fully transparent areas (especially on their borders). It should be perfectly legal to overlap those areas.

@kovidgoyal
Copy link
Owner

Well, the order in which images are composed is undefined if they overlap with the same z-index, so yeah this is a fairly reasonable constraint. That said, adding a non-unique number to placements is not too bad, so I am willing to do it.

@schauveau
Copy link
Author

schauveau commented Dec 1, 2020

I have the feeling that part of the problem could be that the protocol specification does not properly define the terminology. It uses the term 'image' to refer to both the 'image data' and the 'displayed image' (that I call 'graphic element' in my proposal). If you start thinking about them as distinct entities that need to be managed separately then the 'i' refers to an 'image data' entity and then it makes sense to add a similar 'g' index for the 'displayed image' entity.

@kovidgoyal
Copy link
Owner

However, in light of #3111 one might want a way to ensure these are unique as well.

@kovidgoyal
Copy link
Owner

Though maybe just requiring image id and placement number to match is sufficient since id can be made unique.

@schauveau
Copy link
Author

Yes they are unique in my proposal. Attempt to reuse a 'g' identifier is not an error but implies that the previous 'graphic element' is first removed before being drawn. An obvious advantage of that approach is that both operations are happening withing a single escape so no flickering.

@schauveau
Copy link
Author

schauveau commented Dec 1, 2020

Though maybe just requiring image id and placement number to match is sufficient since id can be made unique.

I do not understand what you mean. The current image id 'i' refers to the image data that can be used by multiple 'placements'. They cannot possibly match

@kovidgoyal
Copy link
Owner

I mean that when you are performing any operation on a placement, including creating or deleting one, and you use a number, you must also specify an image id. So in other words a placement is uniquely identified by the tuple (image id, placement number)

@schauveau
Copy link
Author

Not sure why you need the tuple here. The placement number should be sufficient

@schauveau
Copy link
Author

Ho! I see! That is probably because in your implementation the placement list is owned by the image data.

@kovidgoyal
Copy link
Owner

No, read #3111, we want it to be possible for programs that dont know about each other to be able to perform image operations without trampling on each other.

@schauveau
Copy link
Author

That's a difficult problem. I am not really happy with the proposed solution. Replies are tricky to handle properly in TinTin++ because the scripting language is limited. Without patching TinTin++, the best I can do is to declare fixed keyboard escape sequences (like \eOP for F1) but I cannot handle sequences with variable contents. Why not have a single negotiation at the beginning to define a private range of ids?

@kovidgoyal
Copy link
Owner

Then dont worry about it, simply use whatever ids you like, I am
guessing TinTin is an application that uses the full screen, so there is
no question of collision with other images anyway.

@schauveau
Copy link
Author

schauveau commented Dec 1, 2020

Indeed. I used TinTin++ with tmux in the past but this is not my priority today. By the way, another sensible way to support concurrent clients in the graphic protocol would be to allow an optional namespace argument in all escape sequences (in both directions). For instance, a well written program could start by sending a namespace request such as '\e_G?\e\' and kitty should reply with something like '\e_G[abc]\e\' where 'abc' is a new namespace. After that all escape sequences must be of the form `\e_G[abc]...'. The application should of course release its namespace before quitting.

@kovidgoyal
Copy link
Owner

That just means maintaining more global state. There is now the question
of namespace uniqueness and lifetimes. It also means that it becomes
difficult for any program to delete all images on screen if it wants to.

@schauveau
Copy link
Author

There are multiple problems with that last commit.

  1. as I indicated in a previous bug report, replies can be a difficult to handle. In that case, we have to process a reply for each drawing of an image with a placement id. For my use case TinTin++, that's a big NO NO.
  2. I see no good reason for using the tuple placement id + image id. For instance it should be possible to use a placement id on a T action.
  3. When the last placement is removed then the image data is deleted. That cannot be the expected behavior. Only the placement should be removed when 'p' and 'i' are both specified.
  4. I also noticed a bug that was probably there before the commit. After removing an image data, it is not possible to load new images with an id. Loading a new image seems to work fine with an OK reply but any attempt to draw them fails with 'Put command refers to non-existent image with id'

@kovidgoyal
Copy link
Owner

kovidgoyal commented Dec 2, 2020 via email

@kovidgoyal
Copy link
Owner

And just FYI I implemented a way to suppress responses to graphics protocol commands.

@schauveau
Copy link
Author

Cool. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants