-
Notifications
You must be signed in to change notification settings - Fork 19
[POC] Weapon abilities #436
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
base: main
Are you sure you want to change the base?
Conversation
feat: implicit definitions for swords
feat: added OffsetTargeting
|
I think you should raise a discussion on the discord around some of these changes, particular the invulnerability change. I don't disagree with it, but I think it is the sort of thing that should be brought to the group. |
immortius
left a comment
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.
Some thoughts. I'm particular curious about the purpose of cooldown margin.
src/main/java/com/wanderersoftherift/wotr/abilities/ChainAbility.java
Outdated
Show resolved
Hide resolved
src/main/java/com/wanderersoftherift/wotr/abilities/attachment/AbilityCooldowns.java
Show resolved
Hide resolved
| import com.wanderersoftherift.wotr.modifier.effect.ModifierEffect; | ||
|
|
||
| public record CooldownCost(int ticks) implements AbilityRequirement { | ||
| public record CooldownCost(int ticks, int milliticks, int margin) implements AbilityRequirement { |
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.
Perhaps ticks and margin should be a floats. It is a little inconsistent. Not very fussed. Maybe we should change it to milliseconds or seconds for that matter.
Margin is a little odd. I see you have skills with a margin of 3000 for a 9 tick cooldown which is 33% of the range. I'm not 100% on the issue that the margin is solving, is it a network or early input issue? Another option would be a queuing system where an activation can be made early and will be saved until the cooldown is over perhaps?
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 issue I intended to solve with margins is that when you have cooldown of 10 ticks and you get 9% cooldown reduction it won't even make a single tick of difference. That is equivalent to having no cooldown reduction at all. The margin allows activating the ability little early, but the remaining cooldown will get added to the next activation. That way even difference of less than tick can have some effect over multiple activations.
The original intention was to have it always be one tick, but that wasn't very practical. Even Minecraft's internal latency is large-enough that when Client realizes attack is possible, cooldown already fully expired on server. That would probably be even worse on remote server, so for attacking I'm now considering different activation method - client would only send "start atttack"/"stop attack" and server would be responsible for timing. Also for manually activated abilities (or by some normal triggers) it's basically impossible to hit a 1-tick window.
Also I guess it is functionally equivalent to the concept of charge cost I mentioned in the discord ability implementation thread - having margin 2*cooldown would give you 2 extra "charges" on the ability.
| cooldowns = new LinkedHashMap<>(); | ||
| if (data != null && holder instanceof Entity entity) { | ||
| long gameTime = entity.level().getGameTime(); | ||
| long gameTime = getGameTime() * 1000; |
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'm not very convinced there is value in tracking at the subtick level, as opposed to rounding up the cooldown time, though this ties into the question around margin below.
src/main/java/com/wanderersoftherift/wotr/abilities/targeting/OffsetTargeting.java
Outdated
Show resolved
Hide resolved
src/main/java/com/wanderersoftherift/wotr/abilities/targeting/shape/SphereShape.java
Show resolved
Hide resolved
| return TYPE; | ||
| } | ||
|
|
||
| public void handleOnClient(IPayloadContext context) { |
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.
So, what happens if the player changes item to a weapon with a main attack trigger and attacks before this payload arrives?
Well, I assume this will be covered by #180. Presumably the answer is ensuring the client has enough information to set this up client-side without needing to be told by the server (assuming the involved data components are replicated).
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 guess that lets the normal attack through, huh? Should I test whether to override the attack in some other way, then? Or maybe stop the regular attack on server...
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.
Mmm, there's probably not a perfect answer. I guess there could be a specific check client-side for the weapon giving a main attack trigger, since if the main attack trigger comes from somewhere else it won't have the same item switching issue (or it is much more unlikely at least). And yeah, otherwise having the server check for and remove the regular attack.
src/main/java/com/wanderersoftherift/wotr/abilities/effects/ParticleEffect.java
Outdated
Show resolved
Hide resolved
src/main/java/com/wanderersoftherift/wotr/abilities/effects/ParticleEffect.java
Outdated
Show resolved
Hide resolved
…ckOverrideTrigger # Conflicts: # src/main/resources/wotr.mixins.json
updated FieldOfViewTargeting javadoc
refactor: replace OffsetTargeting.x y z with Vec3
Replaced distribution_x, distribution_y, distribution_z with single distribution field Made distribution, speed, count optional
|
Per discussions in design meeting/forum post, we should move forward with the invulnerability change and see how it feels in practice. May lead to future abilities/game settings relating to damage reduction/OHKO protection. |
|
I just playtested this, and it feels very nice. It significantly improved the pacing in the rift. |
…ckOverrideTrigger
feat: weapon abilities now use attack damage
immortius
left a comment
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.
Just a couple of small comments, otherwise looks good to me. Maybe a couple of issues to pull out for future work from the original review.
| tracker.trigger(triggerType.value().clientTriggerInstance()); | ||
| return !tracker.hasListenersOnTrigger(triggerType); | ||
| }); | ||
| triggersToRepeat.forEach(triggerType -> tracker.trigger(triggerType.value().clientTriggerInstance())); |
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.
Is this triggering the triggers twice, since it also happens on line 61?
|
|
||
| public enum CooldownMode implements StringRepresentable { | ||
| NORMAL("normal"), | ||
| INVERTED("inverted"); |
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.
Could use a touch of doco on what the inverted cooldown mode means and when it is intended to be used. I assume it is for when the associated attribute increases to reduces cooldown time.
|
|
||
| private static boolean shouldUseOverrideAttackNew(Player player) { | ||
| var result = new Ref<>(false); | ||
| Arrays.stream(EquipmentSlot.values()).forEach(slot -> { |
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.
anyMatch might be better than forEach here.
Potentially could flat map from EquipmentSlot -> modifierHolder -> modifier and then for each and generally flatten the stream processing that way.
| @Redirect(method = "<clinit>", at = @At(value = "NEW", target = "(Ljava/lang/String;DDD)Lnet/minecraft/world/entity/ai/attributes/RangedAttribute;")) | ||
| private static RangedAttribute redirect(String descriptionId, double defaultValue, double min, double max) { | ||
| if ("attribute.name.attack_speed".equals(descriptionId)) { | ||
| return new RangedAttribute(descriptionId, 1.0, min, max); |
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.
Would be good to capture a comment for why we're overriding the default. I assume it is because by default it is the time of the attack but we're using it as a modifier on the time defined in the attack ability?
POC Weapon abilities
Added MainAttackTrigger
Whenever there's any action registered to this trigger regular attack and breaking blocks become unavailable. It is triggered on repeat when attack is held.
Removed Invulnerability Timer
I belive that negatives of this feature highly outweight any positives. Removing it allows:
Added abilities for Weapon attacks
"Sword": a chain ability of 3 attacks - 2 wide attacks followed by 1 narrower but higher range and damage attack. This is implicit on all swords.
"Dagger": a chain ability of 5 attacks - 4 wide attacks followed by 1 narrower but higher range and damage attack. Faster but lower damage and range than sword.
"Katana": a chain ability of 3 attacks - initial attack with large area and damege followed by 2 fast attacks. The strong initial attack won't be available until you stop attacking, looping the two weaker attacks.
"Spear": single attack - Slow but high damage, narrow but high range.
Other changes
Added some placeholder particles to visualize the different attacks.
Improved
SphereShapeto detect any intersection with entity hitbox instead of just checking the entity origin.DamageEffectnow correctly applies knockback. (closes #384)Added option to configure damage attribute of
DamageEffect.Added
requirementsto chain ability entry, these requirements are applied to the chain ability instead of individual subabilities.Added
nextto chain ability entry, it isIntProviderthat determines the index of next ability to be activated. (mostly closes #367)Various improvements for cooldowns.
Added
FieldOfViewTargeting.Added
OffsetTargetingthat moves position where ability effect happens. (closes #502)Added more configuration options to
ParticleEffect. (closes #505)Reduced default attack speed.
Added
SwingHandAnimation- ability effect that effectively runs/swingcommandRemoved Minecraft's implicits on swords.
Future work
Ability
AnimationEffect- would be pretty nice to have custom animations for individual attacks.More weapon types. (this should include custom mace and trident)
A trigger that would replace right-click.
Improve modifier display.
Make sure all targetings can target blocks and add a way to attack blocks