-
Notifications
You must be signed in to change notification settings - Fork 147
feat: add a plugin to correct all Event.second in lotv #186
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
Conversation
StoicLoofah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so well-versed in the code to remember what all of the other references to frames and times might be that maybe should also be affected. I'm of a mixed mind on this since it is a plugin, so it's optional and presumably safe. However, I do wonder if it might create more maintenance.
ARe you using this yourself and can have the plugin added locally?
| parts = toon_handle.split("-") | ||
|
|
||
| #: The Battle.net region id the entity is registered to | ||
| self.region_id = int(parts[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks unrelated. Is this just something that was missing?
About The PluginThe time record difference between Event and ReplayWhen I was doing an analysis, I found that the Event.second did not match the Replay.game_length. Say I have a replay of a game lasts 8min34s (514s), the last event of the game shows a time record as 720s ( around 514 * 1.4): >>> r = sc2reader.load_replay('../1.SC2Replay')
>>> e = r.events[-1]
>>> r.game_length
Length(seconds=514)
>>> e.second
720Then I dived into the code of time records. For Replay Object, the relevant code seems changed after Lotv: # sc2reader.resources.Replay
...
self.game_fps = 16.0
...
fps = self.game_fps
if 34784 <= self.build: # lotv replay, adjust time
fps = self.game_fps * 1.4
self.length = self.game_length = self.real_length = utils.Length(
seconds=int(self.frames / fps)
)But for Event object, the method to calculate time still use fps as 16.0: # sc2reader.events.game.GameEvent
# The method to calculate time is same among all Event objects
class GameEvent(Event):
"""
This is the base class for all game events. The attributes below are universally available.
"""
def __init__(self, frame, pid):
...
#: The frame of the game that this event was recorded at. 16 frames per game second.
self.frame = frame
#: The second of the game that this event was recorded at. 16 frames per game second.
self.second = frame >> 4This is not good. However, as I mentioned, this situation has been persistent for a long time, and many applications may rely on this. I think the safest way is using a plugin to correct it. Why create a lotv folder inside plugins folder?I found the old plugins seems not working so well now, like apm and supply, but some legacy codebase may still using them. I planned to write some new plugins for lotv replays. This is why I create a "lotv" folder and put the EventSecondCorrector into it. What now?I do use this plugin for my own parser to get all event.seconds right, but I agree with that this might create more maintenance for this codebase. Now I think the better way is creating another repo, so called "sc2reader-plugins", to put this and other my plugins such as SQCalculator there. About one-line-addition to class Entity:This is a function to support look up Observer's url. During my development of my own utility, I found you cannot call observer.url since it does not have needed attribute 'region_id', which can be got easily. So I just add it to my own sc2reader repo. Since I'm fairly new to how github PR works (this is my first PR tho), I did not expect it to be included in this PR, while I probably wanna to fix it in the main repo. So should I just close this PR, and make a new branch in my fork, then fix the Entity.url problem and make a new PR? Any advice and suggestions will be greatly appreciated. |
|
Hi @NumberPigeon , that's great to see you fix this error on the timeline, I also have the same problem. Recently, I tried to do some analysis on data decoded from sc2reader, and I checked to your branch but I have no idea how to use your plugin. I would appreciate it if you could tell me how to use your plugin. Thanks. |
hi @Ivens-Zhang , the usage of plugins is simple: Then all your Event objects should have correct Besides, I decide to move the plugin to another repo, to keep the main sc2reader lib clean, and make manage and updates plugins easier. |
|
So for the slow reply, but exactly to your point, you're probably best maintaining this in a separate repo. I presume that most of the plugins and changes you want are going to be moving more quickly than this repo, and I would hate for you to get blocked on review here for everything you're trying to do on your own. The region tweak looks like a good fix, though! Would be happy to take that one into the repo if you coudl open a separate PR for that. Thanks for contributing! |
All the Events.second now is calculated as frame / 16, which should be 22.4 in lotv. I believe this situation has been persistent for a long time, and many applications may rely on this working method. However, it is not conducive to new development work. I think adding a plugin to correct it might be the best option at the moment.