-
Notifications
You must be signed in to change notification settings - Fork 20
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
Custom Dispenser Behavior addition to station-items-api-v0 #99
Custom Dispenser Behavior addition to station-items-api-v0 #99
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.
Looking on mobile, so I'm not going to review code specifics yet.
You're missing newlines at the end of files, which can make diffs more messy in the future.
Also, maybe make a new cancelable event for when dispensers are triggered and attach the interface functionality as a low priority listener instead, to allow mods more freedom in messing with dispenser behaviour.
.../main/java/net/modificationstation/stationapi/mixin/dispenser/block/DispenserBlockMixin.java
Outdated
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.
My first review.
Also, general feedback - all mixin methods should be private and have the stationapi_
prefix in them to make the logs clearer.
...dispenser-api-v0/src/main/java/net/modificationstation/stationapi/api/item/DispenseUtil.java
Outdated
Show resolved
Hide resolved
...dispenser-api-v0/src/main/java/net/modificationstation/stationapi/api/item/DispenseUtil.java
Outdated
Show resolved
Hide resolved
...dispenser-api-v0/src/main/java/net/modificationstation/stationapi/api/item/DispenseUtil.java
Outdated
Show resolved
Hide resolved
...dispenser-api-v0/src/main/java/net/modificationstation/stationapi/api/item/DispenseUtil.java
Outdated
Show resolved
Hide resolved
.../main/java/net/modificationstation/stationapi/mixin/dispenser/block/DispenserBlockMixin.java
Outdated
Show resolved
Hide resolved
.../main/java/net/modificationstation/stationapi/mixin/dispenser/block/DispenserBlockMixin.java
Outdated
Show resolved
Hide resolved
...t/modificationstation/stationapi/mixin/dispenser/block/entity/DispenserBlockEntityMixin.java
Outdated
Show resolved
Hide resolved
...t/modificationstation/stationapi/mixin/dispenser/block/entity/DispenserBlockEntityMixin.java
Outdated
Show resolved
Hide resolved
...n-items-v0/src/main/java/net/modificationstation/stationapi/api/dispenser/DispenseEvent.java
Outdated
Show resolved
Hide resolved
...s-v0/src/main/java/net/modificationstation/stationapi/api/dispenser/ItemDispenseContext.java
Outdated
Show resolved
Hide resolved
...ems-v0/src/main/java/net/modificationstation/stationapi/api/item/CustomDispenseBehavior.java
Outdated
Show resolved
Hide resolved
...in/java/net/modificationstation/stationapi/impl/dispenser/CustomDispenseBehaviorHandler.java
Outdated
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.
In addition to this: shooting arrows is broken, somehow. They're dispensed as usual items. (intended behavior for testmod, I'm dumb)
public final ItemStack itemStack; | ||
private ItemStack shotItemStack; | ||
public byte xDir = 0; | ||
public byte zDir = 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.
What's the point of making these two non-final if they're populated to the same value always? Also, what's the point of using two bytes when we have the Direction enum?
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, they can be final. The reason for not using the direction enum was straight up so that the code was 1:1 to the existing dispenser code, and I think they should stay this way, because really not much is being lost for it being this way. maybe a byte of memory lol
} | ||
} | ||
|
||
private void populateData() { |
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.
What's the point of this entire method? Can't it be fully moved to the constructor?
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's a good point, I had multiple constructors at one point is why
public int z; | ||
public double xVel; | ||
public double yVel; | ||
public double zVel; |
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.
What's the point of making these six non-final if they're always calculated to the same value?
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.
Sure, I think they can be final
I've tried to make a dispenser modification interface as neatly as possible for an API, I'd be happy to make changes! Here's a demo:
dispenser-demo.mp4