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

Remove old recording APIs. #2825

Closed
wants to merge 1 commit into from
Closed

Conversation

@gw3583
Copy link
Collaborator

gw3583 commented Jun 18, 2018

These are not maintained or used now that we have scene capture
functionality working.


This change is Reviewable

@gw3583
Copy link
Collaborator Author

gw3583 commented Jun 18, 2018

In theory, there is still a use case for the old binary recording system - it supports recording a sequence of frames.

In practice, this seems un-maintained (it didn't work when I tested it today), and I don't intend to use it again now that we have scene capture functionality working reliably.

What do you think @kvark @gankro @mrobinson @jrmuizel ?

These are not maintained or used now that we have scene capture
functionality working.
@gw3583 gw3583 force-pushed the gw3583:remove-old-recording branch from 1cf3426 to 4353ac5 Jun 18, 2018
@gw3583
Copy link
Collaborator Author

gw3583 commented Jun 18, 2018

If / when this lands, we can close the following:

#2583
#2411
#2231
#2131
#2201
#1253
#1402
#1826

@mrobinson
Copy link
Member

mrobinson commented Jun 18, 2018

@gw3583 I'm okay with this. We briefly discussed a better high level API (like the current YAML one) last week, but I think the consensus was that we should be using serde serialization instead of a custom serializer.

@kvark
Copy link
Member

kvark commented Jun 19, 2018

@mrobinson

We briefly discussed a better high level API (like the current YAML one) last week, but I think the consensus was that we should be using serde serialization instead of a custom serializer.

To be fair, the high-level API we discussed is mostly unrelated to the binary recordings. And the binary stuff uses Serde for encoding/decoding (unlike YAML).

@gw3583

In practice, this seems un-maintained (it didn't work when I tested it today), and I don't intend to use it again now that we have scene capture functionality working reliably.

I think a better solution to this problem in the long term would be sequential capturing. Most of the capture time today comes from reading back texture data, and this would be minimal as long as we keep track of what's already on disk and just re-use it. So at the end of the day we'll end up with a sequence of captured frames sharing the resources, and we'll add some controls to wrench to go back and forth between the frames.

So yeah, it does seem like the binary recording can be dropped, even though it's a fairly small chunk of code.

@mrobinson
Copy link
Member

mrobinson commented Jun 19, 2018

To be fair, the high-level API we discussed is mostly unrelated to the binary recordings. And the binary stuff uses Serde for encoding/decoding (unlike YAML).

My comment was just in relation to the bits of this PR that remove the YAML frame writer. I have no reservations at all about removing the binary bits.

@kvark
Copy link
Member

kvark commented Jun 19, 2018

... even though it's a fairly small chunk of code.

...

+1 −2,470

😨

@jrmuizel
Copy link
Contributor

jrmuizel commented Jun 19, 2018

The big advantage of being able to record to yaml is that the output is stable across revisions of WebRender. I would be sad to lose this.

@gw3583
Copy link
Collaborator Author

gw3583 commented Jun 19, 2018

@jrmuizel That's a fair point I hadn't considered. Will ponder this a bit more...

@gw3583
Copy link
Collaborator Author

gw3583 commented Jun 19, 2018

@kvark How feasible do you think it would be to make the scene capture functionality also write out a stable representation of the scene-1-0.ron file? I guess this would semantically be writing a YAML file from the display list in the scene.ron file? That would probably give us a stable representation across WR versions. Or do you have other ideas that might provide that functionality?

@kvark
Copy link
Member

kvark commented Jun 20, 2018

@gw3583 the idea we've been discussing at All Hands is making a semi-stable structures defined that would be serialized to/from instead of RON. That would make the current YAML parser irrelevant, and all the conversion logic will by type-checked properly (but we still need to write/maintain the conversion code from the current internal representation of scenes into that stable definition). This is exactly what #2239 is proposing.

Another (probably simpler, but not as beneficial in the long term) solution would be to attempt to address #2583 (get internal RON -> YAML conversion with Wrench).

@gw3583
Copy link
Collaborator Author

gw3583 commented Jun 22, 2018

Let's close this for now - we probably don't want to remove this completely until we have in place a solution for multi-frame recordings.

@gw3583 gw3583 closed this Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.