-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Synchronize Damage Feature #8305
Conversation
- Added minimum value of 1 for ClientAttackMotion to prevent division by 0 - Changed default value from 432 to AttackMotion
Note: The values for Anubis and probably some other monsters are still incorrect. We are investigating this currently. |
This seems nice :O |
Not sure if md can even be nullptr here, but I guess checking doesn't hurt. Co-authored-by: Atemo <Atemo@users.noreply.github.com>
* Add missing values for cloned or special event type monsters. * Update values for several monsters that was determined incorrect from the sprite parser.
Co-authored-by: Lemongrass3110 <lemongrass@kstp.at>
src/map/battle.cpp
Outdated
// 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) { |
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.
// 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 ) { |
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 understand this change, doesn't this just remove the whole feature of making sychronize_damage work with delay_battle_damage?
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.
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)
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.
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.
src/map/battle.cpp
Outdated
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) |
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.
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) |
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.
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.
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.
Everything above 432 (so 1000) will be ignored by the client as you commented.
So everything is fine.
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 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.
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.
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.
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.
Done!
doc/mob_db.txt
Outdated
|
||
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. |
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 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.
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.
You want documentation to be in the source file rather than the documentation folder?
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.
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.
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 improved the doc to focus on how to set the value rather than how it works internally here.
Co-authored-by: Lemongrass3110 <lemongrass@kstp.at>
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.
Approved with the changes discussed in Discord.
// Many skills show their damage immediately, so setting "delay_battle_damage" to "no" at the same | ||
// time might improve the experience further, but will not work for all skills. |
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.
// Many skills show their damage immediately, so setting "delay_battle_damage" to "no" at the same | |
// time might improve the experience further, but will not work for all skills. | |
// Many skills show their damage immediately, so setting "delay_battle_damage" to "no" at the same | |
// time might improve the experience further, but will not work for all skills. |
Could we not add a check for that as well? Maybe in another PR?
Basically we would just have to replace the skill_id == 0 check with a skill_id == 0 || skill_get_delay == 0 (pseudo) check?
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 correct way to implement this for skills is to define the clientamotion for each skill and then use this information when sychnronize_damage is enabled. Would be great, but currently we don't have this information available.
Co-authored-by: Lemongrass3110 <lemongrass@kstp.at>
Description of Pull Request: