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

Patch defs to work around deficiencies in DIM's plug stat handling #9953

Conversation

robojumper
Copy link
Member

No description provided.

Copy link
Contributor

@bhollis bhollis left a comment

Choose a reason for hiding this comment

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

I like the defensive way you've handled these!

@robojumper robojumper changed the title Patch defs to work around deficiencies in DIM's plug stat handling Patch defs to work around deficeencies in DIM's plug stat handling Oct 6, 2023
@robojumper robojumper changed the title Patch defs to work around deficeencies in DIM's plug stat handling Patch defs to work around deficiencies in DIM's plug stat handling Oct 6, 2023
@nev-r
Copy link
Member

nev-r commented Oct 6, 2023

fully assume you have thought out the answer to this, but for posterity, why isn't at least one of these a case that should be handled with isModStatActive?

@robojumper
Copy link
Member Author

fully assume you have thought out the answer to this, but for posterity, why isn't at least one of these a case that should be handled with isModStatActive?

This will mostly affect DimPlugs, which use isPlugStatActive, and I found some uses of that function that only have access to the plug stats keyed by stat hash (so that we lost index information we'd need to use for this). I'm not quite happy with this PR so I was already experimenting on another branch that cleans up some of our plug stats handling. In that branch I realized that we apparently already eagerly check this when attaching plug stats and never need to call this function later, so you're right, with some more refactoring we should be able to handle this without touching the defs.

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