Skip to content

Allow starting replays by passing them to the launcher as args#91

Open
raph-amiard wants to merge 1 commit intogajop:masterfrom
raph-amiard:topic/launch_replays
Open

Allow starting replays by passing them to the launcher as args#91
raph-amiard wants to merge 1 commit intogajop:masterfrom
raph-amiard:topic/launch_replays

Conversation

@raph-amiard
Copy link
Contributor

No description provided.

@raph-amiard raph-amiard force-pushed the topic/launch_replays branch 2 times, most recently from 512d842 to 7ff4928 Compare May 18, 2021 22:19

const replayPath = decodeURI(url.parse(argv._[0]).pathname);

const engineName = replayPath.split('_').pop().slice(0, -5).toLowerCase();
Copy link
Owner

Choose a reason for hiding this comment

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

is engine name really only contained in the replay file name? can't sdfz-demo-parser be used to parse the actual engine? (is it contained in .sdfz?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it can, but I think parsing the replay to get the engine name is quite wasteful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can write a "better" way to extract it from the filename if you want - like, use a more robust regex and put it in a function ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the end I just removed the fragile extension stripping, using the path module instead for that. IMO it's good enough, tell me what you think

Copy link
Owner

Choose a reason for hiding this comment

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

I am still leaning towards using sdfz-demo-parser after checking whether file exists.

  • I think the "wastefulness" you are referring to (by which I think you mean the extra time it'd take to parse it) is negligible, as the file will have to be loaded from disk anyway, we just cache it a bit earlier.
  • It's a less fragile approach. If a user or script renames a replay file it should still be loadable.
  • It's a way to verify the correctness of the file before trying to feed it to Spring. Trying to load an invalid replay file might have Spring spit out some unintelligible error.
  • We'll have to do it anyway if we want to support downloading missing resources, which is the next step I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All good points indeed. The thing is, sdzf-parser on my machine can take from a few hundred msecs to many seconds (on the order of 10 seconds for really big replays) to parse a replay file - so a long time. The current UI (or lack of it) won't work, because the user will think it crashed/it is not working. I think there are some options in the replay parser to make it faster, but I'd have to see which ones are good for this use case.

I would be for:

  1. Trying to parse the engine name out of the file name, with some more validation (check that it conforms to a given format).
  2. If it fails, then use the sdfz parser, and show the GUI while parsing, so that if/when it takes ~10secs the user has some feedback.

Since this is significantly more work than what I did, if it's OK with you I'd prefer to do this in an additional PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Saw this and added an option to DemoParser to skip the packet parsing. I initially decided against it because it technically means some of the data in the info object can be incorrect or missing, such as player faction or start position, but if you're just trying to get info directly from the demo header, such as engine version, then it should be fine.

const parser = new DemoParser({ skipPackets: true }); in sdfz-demo-parser version 4.5.0

@raph-amiard raph-amiard force-pushed the topic/launch_replays branch from 7ff4928 to 116740e Compare May 20, 2021 11:56
@raph-amiard raph-amiard force-pushed the topic/launch_replays branch from 116740e to 56b0430 Compare May 23, 2021 13:50
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.

3 participants