-
Notifications
You must be signed in to change notification settings - Fork 91
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
Changed GaslightingTracker
to be instanced, not static
#701
base: main
Are you sure you want to change the base?
Changed GaslightingTracker
to be instanced, not static
#701
Conversation
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.
Overall looks like a good start ! needs a bit of cleanup though, and I'd like to get another opinion on whether or not to change the model predicate/how to organize datagen for it, @object-Object if you have opinions.
Common/src/main/java/at/petrak/hexcasting/api/client/GaslightingTracker.java
Show resolved
Hide resolved
Common/src/main/java/at/petrak/hexcasting/common/lib/hex/HexGaslighting.java
Show resolved
Hide resolved
Fabric/src/main/java/at/petrak/hexcasting/fabric/FabricHexInitializer.kt
Show resolved
Hide resolved
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 that method needs to take in a predicate if they all use the same one. or should atleast have a second method that doesn't take in the predicate and just delegates to the one that does.
Also, it seems redundant to take in a resLoc, use that to get the tracker, and then get the resLoc from it? Taking in the GaslightingTracker
itself (since it's a static field) seems like the neatest/most readable imo.
I'm not sure we need to change the actual predicate key though? there's not really a case where an item would have multiple gaslight predicates so I think just keeping it as hexcasting:variant
is fine. If we did this I'm not sure that these would even need to take in anything or be changed much at all?
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.
(nitpick) also be mindful of consistent tabs and newlines, etc
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 know if it'll work for you, but what I've done is have the GaslightingTracker
s be stored with separate keys so that they can have separate timers and cooldowns, but then they all use the same key for the model shenanigans since as you said, there's not really a case for having two.
Common/src/main/java/at/petrak/hexcasting/client/RegisterClientStuff.java
Show resolved
Hide resolved
Just a heads up: we likely will not be merging this or putting more effort into it until after we get a 1.20 release out. It seems to me like a quite specific edge case feature that base Hex doesn't really have a use case for, and the amount of changes makes me wary. I don't have any reason to be against the change, I just want to prioritize the release first. ^^ |
Yea that's absolutely fair, I just made this pr since I had gotten it to work in my addon, so I figured I might as well put it here since it might be useful if a separate gaslighting group was needed at some point. I'll at least get those edits done so it's in a better state if/when it's needed! |
Made
GaslightingTracker
able to be instanced, allowing multiple different gaslighting timers on separate predicatesObviously this isn't too useful in base hexcasting right now as there's only the quenched allay group, but the new class is in api so that it is accessible for addons & the like
There's probably better ways to do some of these things, so any pointers would be great :)