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

Support s2protocol build versions as well. #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frugs
Copy link

@frugs frugs commented Jan 23, 2018

This shouldn't effect any ability to parse Heroes of the Storm replays and adds support for parsing StarCraft II replays.

@jnovack
Copy link
Member

jnovack commented Jan 25, 2018

I'm torn. This looks awesome, but I think it goes outside the scope of this named project.

Either the name of the package is wrong (heroprotocol) and should be changed (sc2engineprotocol or something better named), or the change is in the wrong spot.

I'm not against a package named (sc2protocol, or whatever) along-side of heroprotocol. My OCD doesn't want to run ./heroprotocol.js sc2.replay.

Thoughts?

@frugs
Copy link
Author

frugs commented Jan 25, 2018

I can totally see where you're coming from.

It would be a bit of a shame to not have this merged in however, as I couldn't find any alternative libraries which would work once they were webpacked up to run in a browser. This was the only library which seemed to work, so anyone trying to parse StarCraft II replays using client side javascript would have to either stumble across my fork, or just re-implement the changes I've made here.

Perhaps I can try creating a separate library (named appropriately) which uses heroprotocol as a dependency to achieve what I've done here.

@jnovack
Copy link
Member

jnovack commented Jan 27, 2018

I'm not against merging it, I'm just not FOR it, completely. That's why I wanted to discuss it.

Can you talk me through the differences between sc2replays and hotsreplays? How many files are different?

If this turns into a generic "blizzard-replay-parser" where you pass in --starcraft or --hots (exaggerated example), then we should rename the library (not against it).

I don't want to publish a primary one-or-the-other library with an option for the other. It should either do both equally well, or only one (and another library sc2protocol gets created).

Thoughts?

@frugs
Copy link
Author

frugs commented Feb 5, 2018

Sorry about the late reply!

The majority of data structures in both types of replays seem to be shared, with the difference defined using the two protocols. As the protocols themselves are defined in the same way, nydus/heroprotocol is able to parse sc2replays given a valid protocol definition. As protocol versions are not shared between heroprotocol and s2protocol, looking at the protocol version is sufficient for nydus/heroprotocol to be able to determine which protocol it should use (though perhaps relying on this assumption is inelegant and allowing the user to explicitly specify HotS or SC2 would be more future-proof, robust, and coherent).

With these changes, nydus/heroprotocol should be able to support StarCraft II replays and Heroes of the Storm replays equally well, so perhaps renaming the library might be sensible here; I am aware, however, that renaming an already published library is by no means painless.

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.

2 participants