-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add a warding processor #3
base: master
Are you sure you want to change the base?
Conversation
this processor may be used to track down warding events, such as Counterwarding, Placement or Expiry. It uses the combat log events correlated with entity life state tracking.
@spheenik please have a look at the processor. |
Did you consider submitting this directly to clarity? |
Yes I did, but as opendota would be the first project to use this code I think it could be extracted down the road. I don't know how often you update the clarity's version, so I'd rather have this code here than in |
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 like a lot that finally there's someone using the SpawnsAndDeaths example to good use! ;)
I might be porting the SpawnsAndDeaths processor from clarity-examples to clarity, so that it's events would be available to everyone. Then your Wards-Processor could be simplified.
One thing that came to my mind: What happens if a ward dies of old age, and another one gets countered on the same tick, and the update of the lifestate for the one dying of old age comes first?
I think instead of include the SpawnsAndDeaths, I think a OnFieldPathChanged(Class entityClass, FieldPath path) would be a more useful implementation. Just a suggestion.
Your question is very good, in fact I didn't yet found a way to handle these. I have a replay which represent this exact problem and the output is wrong. This is my next problem to tackle, but I could use your insights on this one.
To me that means the killer of the second sentry is wrong (and the obs too). I'm wondering how frequent same-tick events happens though. |
Hmm, the problem is that the field path differs between different entity types, and there is no way to parametrize the annotation to tell it which fieldpaths you are interested in. One could hack it by doing @OnFieldPathChanged("CDOTA_NPC_Observer_Ward(_Truesight)?", "m_hLifeState")
public void onLifeStateChanged(Matcher matcher, Entity e, FieldPath fieldPath) {
// use matcher to find out if truesight or not,
// and get the current value from the entity using the fieldpath
} |
</execution> | ||
</executions> | ||
</plugin> | ||
<!-- for some reason the shade-plugin doesn't work well with clarity processors. |
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.
preferably we should find a way to avoid needing to swap this dependency.
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 took like 3 hours today trying to make this work. Maybe @spheenik can give some guidance because the shaded jar will crash upon populating the processors. It seems it cannot find the annotations or whatever.
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.
IIRC this produces a jar with a different name. We have to make sure the output file is named the same, or update the Dockerfile to start the correct file.
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.
The problem here is that there is an annotation scanner in clarity (org.atteo.classindex
) that scans annotations on compile time, and produces a file META-INF/annotations/skadistats.clarity.event.Provides
which contains a list of all classes with Provides-annotations. If you use shade, you got to make sure this file is present and correctly merged.
public static void main(String[] args) { | ||
try { | ||
|
||
if (args.length > 0 && args[0].equals("--file")) { System.exit(parse(args[1])); } |
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.
Fix brace spacing
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.
To be honest, if you want to include the Main class, I'd rather have a proper arg-parsing, like Apache Commons CLI or similar.
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.
The current code only has the server case. i assume you added these additional startup methods to make testing easier. Previously, I just started the server and then used curl to POST a replay to it. Up to you if you want to keep them around.
try { | ||
|
||
if (args.length > 0 && args[0].equals("--file")) { System.exit(parse(args[1])); } | ||
else if (args.length >0 && args[0].equals("--")) { System.exit(parseStream(System.in, System.out)); } |
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.
fix >0 spacing
else { startServer(args); } | ||
} | ||
catch (Exception e) { | ||
e.printStackTrace(); |
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.
do you need to catch the exception here? If we have one, we should crash since the startup is incorrect.
} | ||
|
||
static int parse(String replay_file) throws Exception { |
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.
call this parseFile
} | ||
|
||
static int parse(String replay_file) throws Exception { |
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.
use camelCase
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.
and call it fileName
server.createContext("/", new MyHandler()); | ||
server.setExecutor(java.util.concurrent.Executors.newFixedThreadPool(4)); | ||
server.start(); | ||
static int parseStream(InputStream is, OutputStream os) throws IOException { |
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 think methods should have visibility declared.
catch (Exception e) | ||
{ | ||
e.printStackTrace(); | ||
return -1; |
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.
extra space
@OnEntityLeft | ||
public void onEntityLeft(Context ctx, Entity e) { | ||
processEntity(ctx, e, true); | ||
public void onEntityExistenceChanged(Context ctx, Entity e) { |
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.
Since the annotation here is still OnEntityLeft, I think the method name should reflect that.
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.
Actually, if this is only here for cosmetics entities, you should probably just keep the OnEntityEntered and remove OnEntityLeft (assuming ward-related annotated methods will be called separately)
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.
Yeah that could be a good way to catch this.
@@ -459,6 +459,19 @@ public void onTickStart(Context ctx, boolean synthetic) { | |||
} | |||
} | |||
} | |||
|
|||
@OnWardCountered | |||
public void onWardCountered(Context ctx, Entity e, String killer_hero_name) { |
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.
camelCase
|
||
@OnWardExpired | ||
@OnWardPlaced | ||
public void onWardExistenceChanged(Context ctx, Entity e) { |
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.
can you unify this method with the previous one and detect from context which case it is?
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 don't think I can do that as the signatures are different.
} | ||
} | ||
|
||
Entry buildWardEntry(Context ctx, Entity e) { |
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.
add method visibility
entry.key = Arrays.toString(pos); | ||
entry.ehandle = e.getHandle(); | ||
|
||
if (entry.entityleft) |
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.
use braces for all if statements
@Provides({ OnWardCountered.class, OnWardExpired.class, OnWardPlaced.class }) | ||
public class Wards { | ||
|
||
private static final Set<String> WARDS_DT_CLASSES = new HashSet<String>(Arrays.asList( |
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.
possibly add one for Source 1 entity names as well.
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 think there's a shorter way to populate a set with elements that doesn't require Arrays.asList of a new String[].
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.
Yes but in this case .contains is O(1).
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.
yes, you'd keep using a hashset. Unfortunately it doesn't look like set literals are in Java 8: http://stackoverflow.com/questions/10705705/how-do-i-use-collection-literals-in-java-7
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 tried to find the literal notation not to avail.
} | ||
} | ||
|
||
@Initializer(OnWardCountered.class) |
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.
consider changing name to OnWardKilled since that's a more precise description of what happened (a combat log event indicating a killed ward)
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.
Seems legit.
|
||
private static final Set<String> WARDS_TARGET_NAMES = new HashSet<String>(Arrays.asList( | ||
new String[] { | ||
"npc_dota_observer_wards", |
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.
are you sure it's supposed to be plural?
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.
{"time":22,"type":"DOTA_COMBATLOG_DEATH","value":0,"attackername":"npc_dota_hero_abaddon","targetname":"npc_dota_observer_wards","sourcename":"npc_dota_hero_abaddon","targetsourcename":"npc_dota_observer_wards","attackerhero":true,"targethero":false,"attackerillusion":false,"targetillusion":false,"inflictor":"dota_unknown","gold_reason":0,"xp_reason":0}
Don't get me started on Valve's conventions.
this processor may be used to track down warding events, such as Counterwarding, Placement or Expiry. It uses the combat log events correlated with entity life state tracking.
upon a same-tick event, there is a chance that the ward kill will be credited to the wrong attacker, but this commit limits the impact of this caveat by separating the wards from the sentries. thus you will always be credited a ward kill if you last hit a ward
I've finally found time to finish this parser. I've tested on 5 replays with more than 10 counter-wards and I hit 100% success rate. I think this can be merged. I'll start working on the UI. |
You can preview the wardmap here: It currently doesn't support timeline (it just displays all the wards placed when the user is selected). |
Can you sync master to this PR? |
@@ -12,4 +12,6 @@ WORKDIR /usr/src/parser | |||
ADD . /usr/src/parser | |||
RUN mvn -q -f /usr/src/parser/pom.xml clean install -U | |||
|
|||
EXPOSE 5600 |
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.
how did this work before? port option passed via Docker command line?
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 is because the new docker-compose is using named services to access the other services instead of going through the host networking. It is good practice to EXPOSE listening ports on your docker components, it makes it easy to expose the service to other components.
As you can see in docker-compose.yml
environment:
PARSER_HOST: http://odota-parser:5600
POSTGRES_URL: postgresql://postgres:postgres@odota-postgres/yasp
POSTGRES_TEST_URL: postgresql://postgres:postgres@odota-postgres/yasp_test
READONLY_POSTGRES_URL: postgresql://readonly:readonly@odota-postgres/yasp
REDIS_URL: redis://odota-redis:6379/0
REDIS_TEST_URL: redis://odota-redis:6379/1
CASSANDRA_URL: cassandra://odota-cassandra/yasp
CASSANDRA_TEST_URL: cassandra://odota-cassandra/yasp_test
I think docker-compose automatically expose the image EXPOSED ports.
WARDS_TARGET_NAMES = Collections.unmodifiableSet(new HashSet<>(target_by_dtclass.values())); | ||
} | ||
|
||
// simple inderection map |
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.
indirection?
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.
Opps, that's not relevant anymore.
@@ -12,4 +12,6 @@ WORKDIR /usr/src/parser | |||
ADD . /usr/src/parser | |||
RUN mvn -q -f /usr/src/parser/pom.xml clean install -U | |||
|
|||
EXPOSE 5600 | |||
|
|||
CMD ["java", "-jar", "-Xmx256m", "/usr/src/parser/target/stats-0.1.0.jar", "5600"] |
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.
will you need to update this for the jar name?
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.
No, I've changed the pom.xml to output the same filename as before.
@spheenik could you give this another once-over? |
@spheenik I'm not using the new stuff you've pushed to clarity 2.2, I'll wait for opendota to catch up the version before doing this. |
You can bump the version to 2.2 in this PR. I don't believe there are any breaking changes. |
|
||
private Entry buildWardEntry(Context ctx, Entity e) { | ||
Entry entry = new Entry(time); | ||
boolean isObserver = !e.getDtClass().getDtName().contains("TrueSight"); |
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.
Extra space
Needs to be rebased against master (and I think clarity 2.2 has some functionality that can make this PR simpler) |
Please consider this as a draftThis processor may be used to track down warding events, such as
Counterwarding, Placement or Expiry. It uses the combat log events
correlated with entity life state tracking.
Sample log output:
I still need to test for controlled minions counterwarding.
Feel free to add your thoughts.