Synchronize Damage Feature #8305

Playtester · 2024-05-04T18:25:47Z

Addressed Issue(s): Movement desync #259

Server Mode: Both

Description of Pull Request:

Added a new monster stat "ClientAttackMotion" to mob_db.yml which is the time from when a monster attacks until which the damage shows on the client at 1x speed
Added a new config synchronize_damage; when set to "yes", the client will display the damage of normal monster attacks at the exact time it is applied on the server, removing position lag (fixes Movement desync #259)

* Fixes #259. * Initial attempt to resolve position lag with normal attacks against monsters.

- Added a new config synchronize_damage, when set to "yes", the client will display the damage of normal monster attacks at the exact time it is applied on the server, removing position lag (fixes #259)

- Added minimum value of 1 for ClientAttackMotion to prevent division by 0 - Changed default value from 432 to AttackMotion

Playtester · 2024-05-04T20:09:16Z

Note: The values for Anubis and probably some other monsters are still incorrect. We are investigating this currently.

src/map/status.hpp

src/map/battle.cpp

src/map/clif.cpp

src/map/mob.cpp

SapitoSucio · 2024-05-05T08:54:20Z

This seems nice :O

Not sure if md can even be nullptr here, but I guess checking doesn't hurt. Co-authored-by: Atemo <[email protected]>

* Add missing values for cloned or special event type monsters. * Update values for several monsters that was determined incorrect from the sprite parser.

src/map/battle.cpp

src/map/clif.cpp

src/map/mob.cpp

Co-authored-by: Lemongrass3110 <[email protected]>

conf/battle/client.conf

src/map/clif.cpp

Lemongrass3110 · 2024-05-21T18:26:13Z

src/map/battle.cpp

+ // Check for delay battle damage config
+ // If the synchronize damage feature is activated then disabling the config should only affect skills (for monsters)
+ if (!battle_config.delay_battle_damage && (!battle_config.synchronize_damage || skill_id != 0 || src->type != BL_MOB))
+ amotion = 1;
+ // If clientamotion is 1 or lower for monsters, we can apply damage directly for normal attacks and don't need to create a timer
+ else if (battle_config.synchronize_damage && skill_id == 0 && src->type == BL_MOB && status_get_clientamotion(src) <= 1)
+ amotion = 1;
+
+ // Skip creation of timer
+ if (amotion <= 1) {


Suggested change

// Check for delay battle damage config

// If the synchronize damage feature is activated then disabling the config should only affect skills (for monsters)

if (!battle_config.delay_battle_damage && (!battle_config.synchronize_damage || skill_id != 0 || src->type != BL_MOB))

amotion = 1;

// If clientamotion is 1 or lower for monsters, we can apply damage directly for normal attacks and don't need to create a timer

else if (battle_config.synchronize_damage && skill_id == 0 && src->type == BL_MOB && status_get_clientamotion(src) <= 1)

amotion = 1;

// Skip creation of timer

if (amotion <= 1) {

// The client refuses to display animations slower than 1x speed

// So we need to shorten AttackMotion to be in-sync with the client in this case

if( battle_config.synchronize_damage && skill_id == 0 && src->type == BL_MOB && amotion > status_get_clientamotion(src) ){

amotion = status_get_clientamotion(src);

}

if ( !battle_config.delay_battle_damage || amotion <= 1 ) {

I don't understand this change, doesn't this just remove the whole feature of making sychronize_damage work with delay_battle_damage?

Either you want to delay or you want to synchronize. You cannot have both.
If whoever enables your new setting, they want synchronized damage and not delayed damage.
So instead of hiding that we give the new feature priority, we just straight forward go for it.

(And additionally we dont touch the old feature - we may have to find out, when it was broken - or if it has always been for any damage, but outside of the scope of this PR)

Hmm, but it's important to give the player the option to combine the two settings. Basically battle_sychronize_damage only affects normal skills so of course if you enable this you want normal attacks to be synchronized, which should take priority over the other feature (I'll make it clearer in the description that it takes priority).

At the same time, we want to give the user the choice if they want to delay skill damage (and player damage) or not.

The current implementation from me does not affect the other feature as long as you keep it disabled, so it isn't a problem of backward compatibility.

Lemongrass3110 · 2024-05-21T18:26:56Z

src/map/battle.cpp

+ if (battle_config.synchronize_damage && skill_id == 0 && src->type == BL_MOB && amotion > status_get_clientamotion(src)) {
+ // The client refuses to display animations slower than 1x speed
+ // So we need to shorten AttackMotion to be in-sync with the client in this case
+ amotion = status_get_clientamotion(src);
+ }
+ else if (src->type != BL_PC && amotion > 1000)


Suggested change

if (battle_config.synchronize_damage && skill_id == 0 && src->type == BL_MOB && amotion > status_get_clientamotion(src)) {

// The client refuses to display animations slower than 1x speed

// So we need to shorten AttackMotion to be in-sync with the client in this case

amotion = status_get_clientamotion(src);

}

else if (src->type != BL_PC && amotion > 1000)

if (src->type != BL_PC && amotion > 1000)

We need to put the if(battle_config.synchronize_damage [...]" part here because otherwise you'd cap amotion to 1000 even if the feature applies.

Everything above 432 (so 1000) will be ignored by the client as you commented.
So everything is fine.

I think you are missing that the amotion is something completely different from the value we want to send to the client.

Amotion is the time in milliseconds until which the damage shows (here it's only used to create the timer). This isn't limited to 432.

What is (technically) limited to 432 is the what we send to the client, but this is not a value in milliseconds but rather the inverted animation speed.

One monster where this is relevant would be Medusa:

AttackMotion: 1320 ClientAttackMotion: 1080

Here we want amotion in battle.cpp to be 1080.

The value we send to the client would be 432 (or higher), so it plays at 1x speed which results in 1080ms until the damage shows.

Actually after sleeping about it, why don't we move the 1000 limit also to the top? Then we can simplify the code as you suggested without breaking the feature.

If I look at the code "amotion" is not used anywhere except the <=1 check and for the timer at the end.

src/map/clif.cpp

Lemongrass3110 · 2024-05-21T18:47:13Z

doc/mob_db.txt

+
+Background:
+When a clif_damage packet is sent to the client it will also send "sdelay" (AttackMotion) as value.
+The client will ignore all values above 432, and 432 stands for 1.0x animation speed. 216 for example means play the animation at double the speed.


I know what you are trying to say, but reading this explanation here gives the impression that I should set ClientAttackMotion to a maximum of 432 and if I want to double its speed I should set it to 216.
But in reality you are talking about something totally different.
Please consider moving this (useful) documentation into clif, as it may be confusing for "normal" users here.

You want documentation to be in the source file rather than the documentation folder?

Well not 100% sure, but since it has nothing to do with "to what should i set that value" I was thinking that may be the better place.

I improved the doc to focus on how to set the value rather than how it works internally here.

conf/battle/client.conf

Co-authored-by: Lemongrass3110 <[email protected]>

aleos89 and others added 3 commits May 2, 2024 14:04

Adds ClientAttackMotion to mob database

059f6e0

* Fixes #259. * Initial attempt to resolve position lag with normal attacks against monsters.

Increased version number

e89d89e

Synchronize Damage Feature

82afe60

- Added a new config synchronize_damage, when set to "yes", the client will display the damage of normal monster attacks at the exact time it is applied on the server, removing position lag (fixes #259)

Playtester self-assigned this May 4, 2024

ClientAttackMotion Min and Default

fae8477

- Added minimum value of 1 for ClientAttackMotion to prevent division by 0 - Changed default value from 432 to AttackMotion

Resolves invalid parsed values

2b02765

aleos89 reviewed May 4, 2024

View reviewed changes

src/map/status.hpp Show resolved Hide resolved

Atemo reviewed May 4, 2024

View reviewed changes

src/map/battle.cpp Outdated Show resolved Hide resolved

src/map/clif.cpp Outdated Show resolved Hide resolved

src/map/mob.cpp Show resolved Hide resolved

Playtester and others added 6 commits May 5, 2024 12:25

Made synchronize_damage work together with delay_battle_damage

d30e016

Apply suggestions from code review

ef28d9e

Not sure if md can even be nullptr here, but I guess checking doesn't hurt. Co-authored-by: Atemo <[email protected]>

Ensure clientamotion is at least 1 already in constructor

3bc4534

Update ClientAttackMotion values

70fdcba

* Add missing values for cloned or special event type monsters. * Update values for several monsters that was determined incorrect from the sprite parser.

ClientAttackMotion cleanup part deux

31ad1f4

Added status_get_clientamotion and optimized implementation

1262acd

Lemongrass3110 requested changes May 20, 2024

View reviewed changes

Playtester and others added 3 commits May 20, 2024 21:30

Apply suggestion: Check config first

611e2c3

Co-authored-by: Lemongrass3110 <[email protected]>

Code optimizations based on feedback from Lemongrass

6586b56

Fixed spaces

36e7923

Lemongrass3110 requested changes May 21, 2024

View reviewed changes

Playtester and others added 4 commits May 21, 2024 21:55

Removed != 0 check

f1430ba

Co-authored-by: Lemongrass3110 <[email protected]>

Improved documentation and moved setting to battle.conf

2fc675b

Fixed problem that caused build to fail

52fdaf7

Unified the amotion calculation in battle_delay_damage

a801950

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize Damage Feature #8305

Synchronize Damage Feature #8305

Playtester commented May 4, 2024

Playtester commented May 4, 2024 •

edited

SapitoSucio commented May 5, 2024

Lemongrass3110 May 21, 2024

Playtester May 21, 2024

Lemongrass3110 May 21, 2024

Playtester May 21, 2024

Lemongrass3110 May 21, 2024

Playtester May 21, 2024

Lemongrass3110 May 21, 2024

Playtester May 21, 2024

Playtester May 22, 2024

Playtester May 22, 2024

Lemongrass3110 May 21, 2024

Playtester May 21, 2024

Lemongrass3110 May 21, 2024

Playtester May 21, 2024

Synchronize Damage Feature #8305

Are you sure you want to change the base?

Synchronize Damage Feature #8305

Conversation

Playtester commented May 4, 2024

Playtester commented May 4, 2024 • edited

SapitoSucio commented May 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Playtester commented May 4, 2024 •

edited