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

[RFE] Protocol lacks a means to forget tracks #99

Open
vcaputo opened this issue Aug 10, 2023 · 11 comments
Open

[RFE] Protocol lacks a means to forget tracks #99

vcaputo opened this issue Aug 10, 2023 · 11 comments

Comments

@vcaputo
Copy link
Contributor

vcaputo commented Aug 10, 2023

In my project that's a kind of meta-demo you interactively create and rearrange scenes through a creative process, the Rocket integration automatically creates tracks as the scenes are created/removed/reconfigured.

This mostly works already, but as the creative process progresses the tracks just keep accumulating, even those which are no longer needed. When using emoon's RocketEditor it even leads to crashes because the obsoleted tracks never go away and eventually break some finite assumptions. (I've already landed a fix upstream there to mitigate the crashing issue)

I'd like to add something like FORGET_TRACK to the protocol and library, which the Demo would send the Editor. It'd just have the track id as a parameter, as a way of reporting to the Editor "this track I previously performed a GET_TRACK on and we allocated this id for, I'm discarding it and all its rows from my internal state, any keys sent for this ID now I'll be dropping on the floor". If we had no interest in recycling the track id, that would be sufficient already just to get the Editor to reclaim the relevant UI resources/visual space.

Things get more complicated if recycling the track ids of forgotten tracks is desired, due to how the track ids are implicitly allocated via GET_TRACK like an AUTO_INCREMENT mechanism. For recycling I think we'd need an explicit RECYCLE_TRACK which would be like GET_TRACK with the addition of the track id # to be recycled. Then the subsequent SET_KEY storm would come back out with the named track's contents, using the specified track id. So the Demo would initiate the recycle from its side, by knowing it has a forgotten track id to put to use it previously sent FORGET_TRACK for.

I haven't coded up anything in this vein yet, but I already have a substantial project onhand that could serve as a test bed for this functionality without too much effort.

Curious what upstream thinks of this, before I go too far on the implementation side of this.

@kusma
Copy link
Member

kusma commented Aug 17, 2023

So I've had some thoughts about similar-ish mechanisms in the past, and I have some reservations against an explicit FORGET_TRACK command, for a few reasons.

First of all, right now, the editor "owns" all the data. Having a client request a track to be forgotten turns that on head, but only partially. I think that would make things even more confusing than it already is ;)

But also, allowing a client to remotely delete data means that we'd need to be much more paranoid about security. Imagine someone port-scanning on the default rocket port at a demo party and attempting to tell the editor to throw away data. That would be quite catastrophic to the people making entries using rocket.

But yes, it would be nice to have some way of pruning unused tracks. I do believe this would belong more in the editor than in the client, though. One thing that I've been considering, is for to the editor to mark a bitmap of what track was read since the last update, and communicate that to the editor. That would be useful for a few different use-cases:

  1. We could visualize "active" tracks in the editor, and let an artist could see what tracks they can expect to have effect. Our demos often do a lot of logical branching, leaving tracks unused for large parts of the demo, and the musician is often frustrated when trying to use things that isn't hooked up in the current code-path.

  2. We could automate playing back the whole timeline and collecting all tracks read so we could garbage collect what's no longer in use.

  3. This would also give us a "pulse" from the player each frame, which seems generally useful. We e.g could display the frame-rate of the demo somewhere, meaning demos can get a frame-rate display without having to add it to their rendering code (which has the added down-side of slowing down the rendering even more, making the displayed frame-rate less accurate)

But, changes like this are unfortunately going to break backwards compatibility, which would be a problem for stuff like Emoons RocketEditor and all the other ports out there, which is why I haven't moved forward with this yet.

In general, we really need some sort of capability negotiation in the protocol, but going through all the different implementations and finding a "hole" where we could plumb it through for all of them seems like a large task where we could come out empty-handed anyway...

@kusma
Copy link
Member

kusma commented Aug 17, 2023

In general, we really need some sort of capability negotiation in the protocol, but going through all the different implementations and finding a "hole" where we could plumb it through for all of them seems like a large task where we could come out empty-handed anyway...

Actually... thinking about it again, I think I see how we could "upgrade" the protocol without breaking old protocols; the SET_ROW command. It's entirely unbounded, and since it's a command that goes in both directions, we should be able to communicate a "hey, I'd like to upgrade this connection" sequence where the other end would have to reply with the right sequence back to initiate the upgrade.

That way, each side could send some sort of list of protocol features, and have nice-to-have features such as this in there. And we could take the intersection of those features as the active feature-set in use.

Dunno, it's kinda convoluted, but could work.

@vcaputo
Copy link
Contributor Author

vcaputo commented Aug 17, 2023

Is it really necessary to do such awkward things for determining capabilities?

There's already a greeting handshake, all we'd need is for the Editor's reply to include the version/capabilities instead of just saying "hello, demo!". The library could just examine the Editor's reply, if it's "hello demo!".. it's classic Rocket, if it's "v??? hello demo!" then it's a versioned reply and you can determine what the editor can handle from the reply.

Then it's just a matter of defining what goes into the versions, and eventually the editors catch up and start replying with the version they're at.

I'm not convinced on the security concerns. The Editor has a lot of options at its disposal in how it actually handles FORGET_TRACK, it's primarily just a hint from the demo telling the Editor it can remove it from cluttering up everything because the demo no longer uses it. Just don't bother implementing track id recycling, then the mapping can persist, there's plenty of id space so it's not really critical to recycle them. I'd probably want the Editor to move forgotten tracks into a sort of trash bin pattern, in case there's a forgotten track I want to copy and paste the pattern from into a different one. It's definitely not something I'd want done in a potentially lost data fashion.

@kusma
Copy link
Member

kusma commented Aug 17, 2023

Is it really necessary to do such awkward things for determining capabilities?

There's already a greeting handshake, all we'd need is for the Editor's reply to include the version/capabilities instead of just saying "hello, demo!".

I'm afraid so :(

What you're proposing would break basically all old versions of the library (and all the ports), which will close the socket in when seeing something else than "hello, demo!".

@kusma
Copy link
Member

kusma commented Aug 17, 2023

I'm not convinced on the security concerns.

Sure, the security concerns doesn't have to be a problem with all possible implementations. But it does invite scary behavior, which I don't think is the correct semantics here.

I also think that the whole track ID recycling issue kinda hints that this isn't the right direction to take.
(edit: changed my mind on this one, see here)

Just as a side-note (not really against or for any of this): Unused tracks have little performance overhead, and if we want to reclaim the (usually relatively small amount of) memory we could always just disconnect and reconnect...

@kusma
Copy link
Member

kusma commented Aug 17, 2023

I guess if we saw GET_TRACK and FORGET_TRACK as something more along the lines of SUBSCRIBE_TRACK_CHANGES and UNSUBSCRIBE_TRACK_CHANGES mental model, things would make a bit more sense... (edit: unless it's unclear, this is me changing my mind, realizing I just need to apply a different mental model, which is kinda already the case here)

That way, it's clear that this just means to stop getting updates. The ID could be recycled without worry, because subscribing would assign a new one anyway, and they only need to be unique per connection.

@kusma
Copy link
Member

kusma commented Aug 17, 2023

Thinking about it, I don't think the ID recycling issue is a real issue in the first place. The IDs are already per connections, they're more like a "subscription ID" than a "track ID" in the first place...

@kusma
Copy link
Member

kusma commented Aug 17, 2023

Sorry for the scatter-brain activity here, but to try to complete the circle:

Yeah, I think this could work. I think the semantics here would need to be clearer than just "forget", but "get" is already kinda unclear so, 🤷...

But the protocol compatibility is the only thing that really worries me. That's something that I've been meaning to clean up for at least a decade, just never gotten around to doing it properly...

@vcaputo
Copy link
Contributor Author

vcaputo commented Aug 17, 2023

Reuse is just tricky because the lifecycle of the track-name<->ID mapping is unclear. It's kind of similar to the PID reuse problem in *NIX.

The trouble is GET_TRACK, despite sending just the track name, doesn't receive the ID mapping from the Editor. The Demo, via librocket, independently determines the ID mapping just by using an allocation strategy in common with the Editor. So the Editor replies to GET_TRACK with SET_KEYs at an ID arrived without any synchronous acknowledgement first, and will continue to do so until all the track data is sent, or whenever a key is changed for that track. This works fine where there's no possibility for ID reuse by a different track.

Imagine the Demo sends FORGET_TRACK while the Editor is still sending SET_KEY values for the now forgotten mapping. If we don't do any ID reuse, handling this is trivial for the Demo - it can just mark that ID as "forgotten" and drop any subsequent SET_KEYs received for it on the floor. The ID is burned from the Demo's perspective the moment FORGET_TRACK was sent, now it's just a matter of the Editor catching up. What the Editor does with the keys for the forgotten track is its problem, but it should probably keep them around somewhere just in case.

But if the IDs are reused, and let's say another unique GET_TRACK happens immediately after a FORGET_TRACK, if the mapping allocation strategy is to always use the lowest available ID, the subsequent GET_TRACK would use the just-forgotten ID. So any in-flight SET_KEYs could very well be intended for the forgotten track, and now be getting applied to the track on the reused ID.

This is all because the two parties don't coordinate the mapping allocation at all, so there's no way for them to synchronously get on the same page when a reuse occurs. I think the simplest robust solution is to just consume the IDs and maybe name this RETIRE_TRACK instead, now that I've looked into this more and prototyped a bit.

I suspect this might all seem a little weird because the usual use of Rocket is a largely static Demo implementation with a relatively fixed set of tracks that don't evolve much at runtime. But my project is more like a meta-demo or demo engine, where the creative process involves interactively reconfiguring scenes from components, each exposing their own variables that get bound to Rocket tracks named procedurally according to their logical place within the scene, kind of like filesystem paths. You can probably imagine how that quickly accumulates a lot of stale tracks in the Rocket Editor as one iterates on a given scene, moving modules around with their paths (track names) getting adjusted with every iteration.

Ideally we'd support FORGET_TRACK and be able to robustly reuse the IDs, but that seems tricky within the current protocol. RETIRE_TRACK where the ID allocation persists seems 100% fine within the current protocol, and it wouldn't be too painful if a disconnect->reconnect were required when IDs were exhausted. The Editor could cleanup its stash of RETIRE_TRACK data on a reconnect, or at least ask the user if it should. I don't expect to exhaust the ID space, but with more automation assisting in the creative process, it becomes a possibility.

@kusma
Copy link
Member

kusma commented Aug 17, 2023

Ugh, you're right. I had forgotten about that intricacy :(

Yeah, perhaps RETIRE_TRACK + disconnect->reconnect is the right semantics...

@vcaputo
Copy link
Contributor Author

vcaputo commented Aug 17, 2023

Now we just need mechanism for versioning the protocol that doesn't break existing users...

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

No branches or pull requests

2 participants