Poe zone tracker service #63
Conversation
… more testing. Needs more robustness in general
| .character(charName) //TODO Find a solution to call this function to get non-stale data, probably more intelligent rate-limiter | ||
| .pipe( | ||
| filter((e) => e.type == 'result'), //MAKE SURE ERRORS ARE HANDLED ASWELL. PROBABLY USE OF + ZIP TO CHECK CURRENT ZONE IS STILL THE SAME IN CASE OF RATELIMIT | ||
| map((e) => e as SmartCacheResultEvent<PoeCharacter>), |
There was a problem hiding this comment.
Is this really the way to get typescript to recognize the type, or am i missing something?
There was a problem hiding this comment.
194:196 can be replaced with validResults from https://github.com/PoeStack/poestack-sage/blob/main/src/echo-common/src/smart-cache-hooks.ts#L25
There was a problem hiding this comment.
validResults has a more strict type definition so typescript will recognize the types.
|
Is the build failing something that is currently being resolved in another branch? |
| @@ -32,6 +37,7 @@ export function buildContext(contextSource: string): EchoContext { | |||
| plugins: ECHO_PLUGIN_SERVICE, | |||
| poeAccounts: POE_ACCOUNT_SERVICE, | |||
| poeClientLog: POE_CLIENT_LOG_SERVICE, | |||
| PoeZoneTracker: POE_ZONE_TRACKER_SERVICE, | |||
There was a problem hiding this comment.
I think the casing is wrong here
| @@ -12,5 +12,5 @@ | |||
| "jsx": "react-jsx", | |||
| "sourceMap": true | |||
| }, | |||
| "include": ["./src"] | |||
| "include": ["./src", "./test"] | |||
There was a problem hiding this comment.
@kongsgaard this change is what is breaking the build.
When you add this folder, it changes the paths that the build generates (adds a /src and /test
If you update the package exports here and here with the new generated paths, everything works
| let tail: Tail | ||
|
|
||
| beforeEach(() => { | ||
| const filename = Math.floor(Math.random() * 999999999999) + '.txt' |
There was a problem hiding this comment.
if there isn't a reason these need to be numbers it would probably be better to use https://www.npmjs.com/package/uuid I think we already have this as a dep everywhere.
There was a problem hiding this comment.
I added and now use the uuid package, it was not originally used in echo-common
| .subscribe((event) => { | ||
| if (this.currentZoneDelta === undefined) { | ||
| //Emit as event | ||
| throw new Error( |
There was a problem hiding this comment.
Throwing errors inside a subscribe like this will make it impossible to catch them anywhere. That might be okay if these are errors that should never happen, but even then since they are uncatchable converting them to just logging warnings/errors probably makes more sense.
There was a problem hiding this comment.
Created another subject which emits errors. Maybe the service should temporarily timeout on such an event, to avoid spamming errors, but i am not entirely sure on the best way of handing these "errors"
| this.currentZoneDelta.deltaLoadTime = event.systemUptime - loadStart | ||
|
|
||
| if (this.characterService != undefined) { | ||
| const charName = 'BlaesendeBruno' //TODO from settings |
There was a problem hiding this comment.
We probably want to use the character with current set to true from the character list as a general solution, or make this class more of a util than a context() object. If it was constructed by the plugins themselves they could pass in and manage what character to track themselves.
There was a problem hiding this comment.
I removed the character based stuff for now, as it was not properly ready to be merged into main and be used elsewhere
| filter((e) => e.type == 'NPCEncounterEvent'), | ||
| map((e) => e as PoeNPCEncounterEvent) |
There was a problem hiding this comment.
Checks like these could be converted to if(e.type !== "XXXXX") return; in the subscribe, since the filter/map aren't doing anything special in the pipe.
There was a problem hiding this comment.
Seems like a better way to handle it in general yes
|
I updated the pull-request based on the review. I removed the half-baked character based stuff, as it was not entirely ready to be used. I have stashed it and will work on making it more solid, and get it into service once some other elements (such as a more centralized 'current character') is in place |
|
Btw this is ready to be re-reviewed, or merged |
Added a poe-zone-tracker-service, which aggregates from the poe-client-log-service into zones.
Information is aggregated in a PoeZoneDelta type, which represents the things that happened within entrance to exit of a zone. PoeZoneInstances represents an actual instance of a zone in PoE, so one PoeZoneInstance can have multiple PoeZoneDeltas, if the zone was entered/exited multiple times.
Currently hideouts and towns have individual PoeZoneInstance objects per enter/exit.
Two subjects are exposed, zoneEntered$ and zoneExited$. The PoeZoneDelta will be the same object, just with more information when emitted from the zoneExited$ event stream.
The character features are removed from this PR, and will be developed later
The character snapshot/delta is currently not working fully. Partially due to a long default stale_ms on the smart_cache, and because the delta for stuff like curreny stack-sizes are not calculated currectly. So if a chaos orb stack was incremented from 2 to 5, it will not show up in the inventoryDelta, as these items still has the same id.
In general, alot more utilities for calculating inventory/stash diff of literal items over time should be implemented.
I will write an .md file for both the poe-client-log-service and the poe-zone-tracker-service, to document usage. After that i will try to make the zone-tracker more robust to errors.