[core, lua] Improve draw-in specification to fix some issues #6007

TracentEden2 · 2024-07-14T19:42:37Z

I affirm:

I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
I have read and understood the Contributing Guide and the Code of Conduct.
I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

This PR changes the specification of draw-in functions in several places to support flexibility and the additional options needed for the draw-in logic of different mobs.

Current LSB draw-in problems:

The DRAW_IN mob mod is currently representing two different options that do not always occur together. Specifically, a DRAW_IN > 1 indicates alliance draw-in and a DRAW_IN > 1 also sets the draw-in trigger distance to the mob mod. Thus all mobs with a non-standard trigger distance are also forced to have alliance draw-in. This creates several draw-in bugs.
No ability to draw-in to the front of the mob (as opposed to mob center)
No ability to set a max draw-in distance (beyond which a player or alliance member will not be draw-in)
The lua draw-in function (used for ad-hoc draw-ins in special case mobs) does not allow any options and does not allow overriding the mob or hard coded draw-in defaults.

Changes by this PR:

Changes the DRAW_IN mob mod to be a bitmask mod that represents several different draw-in options including perform normal draw-in logic, include alliance members in draw-in, and draw-in to front of mob. Also adds mob mods MOBMOD_DRAW_IN_TRIGGER_DIST and MOBMOD_DRAW_IN_MAX_RANGE to allow specifying the draw-in trigger distance and the max draw-in range. The bitmask approach allows flexibility and easy future expansion to new draw-in types that LSB is still missing.
Changes the lua draw-in function to allow options as parameters. The order of precedence is thus: specified function option -> mob mod(s) -> hardcoded default option
Changes mobs with draw-in to use the new and updated draw-in mob mods

Steps to test these changes

Fight mobs with draw-in and vary the draw-in mob mods to set different options, also test the lua drawIn function call with different parameters

Xaver-DaRed · 2024-07-14T19:48:04Z

Why not change draw in to mean 1 -> target player + pets, and 2 for party/alliance, (so no numbers over 2) and add another mobmod for draw in trigger distance, without the need of having 4 local variables?

And actually, the original draw in could be changed to set draw in type (player or party / to in front of mob OR to specified locations, like Khaimaira)

TracentEden2 · 2024-07-14T20:02:47Z

Why not change draw in to mean 1 -> target player + pets, and 2 for party/alliance, (so no numbers over 2) and add another mobmod for draw in trigger distance, without the need of having 4 local variables?

And actually, the original draw in could be changed to set draw in type (player or party / to in front of mob OR to specified locations, like Khaimaira)

The idea is there are a lot of potential differences between different draw-ins so would need to have several more mob mods as the dimensions are at least: alliance, in front, trigger dist, max dist, type (leash vs. no leash), include dead/mounted, etc. For brevity not all of these are included in this PR, leash and others will come later on. Also the local variables make understanding mob draw-in logic easier to understand from just the mob file (without needing to look through other logic).

Xaver-DaRed · 2024-07-14T20:11:29Z

Since you are already reducing the original mobmod to a bit, one could use the original mobmod as a bitmask.
original mobmod (type)
bit 0 -> has draw in
bit 1 -> include alliance (vs target only)
bit 2 -> specified pos (vs in front)
bit 3 -> include dead
bit 4 -> include mounted
bit 5 -> leash (i don even know what this is)
..and so on

New mobmod
draw in trigger distance

This are just ideas. I'm personally not a fan of abusing the use of local variables for global systems

TracentEden2 · 2024-07-14T20:31:53Z

Since you are already reducing the original mobmod to a bit, one could use the original mobmod as a bitmask. original mobmod (type) bit 0 -> has draw in bit 1 -> include alliance (vs target only) bit 2 -> specified pos (vs in front) bit 3 -> include dead bit 4 -> include mounted bit 5 -> leash (i don even know what this is) ..and so on

New mobmod draw in trigger distance

This are just ideas. I'm personally not a fan of abusing the use of local variables for global systems

Yeah, could maybe do it with a bitmask and only three new mob mods: trigger distance, max distance, and leash distance, however we lose the readability and explicitness and have to rely on potential code comments. In the end I do not mind either method, but I guess depends on what should be prioritized.

Leash draw-in is where the mob only allows the players to move a specified distance away from a fixed center point of a shape before drawing in potentially to a center point or other point in that shape, thus the mobs never leaves that shape (like dog on leash), this is like Aspid for example (note aspid also has normal non-leash draw-in it can use inside the circle)

TracentEden2 · 2024-07-26T23:04:37Z

Since you are already reducing the original mobmod to a bit, one could use the original mobmod as a bitmask. original mobmod (type) bit 0 -> has draw in bit 1 -> include alliance (vs target only) bit 2 -> specified pos (vs in front) bit 3 -> include dead bit 4 -> include mounted bit 5 -> leash (i don even know what this is) ..and so on

New mobmod draw in trigger distance

This are just ideas. I'm personally not a fan of abusing the use of local variables for global systems

Ok, I updated the PR to use a bitmask mob mod for DRAW_IN and two new mob mods for setting the trigger distance and max range :)

WinterSolstice8 · 2024-07-28T23:05:55Z

CI has a real complaint saying PTarget is not used in a lambda

TracentEden · 2024-07-29T12:47:55Z

CI has a real complaint saying PTarget is not used in a lambda

Thanks for pointing this out, I have updated the PR now!

zach2good · 2024-08-16T08:42:26Z

scripts/enum/mob_mod.lua

@@ -19,7 +19,7 @@ xi.mobMod =
    HP_HEAL_CHANCE         = 9,  -- can cast cures below this HP %
    SUBLINK                = 10, -- sub link group
    LINK_RADIUS            = 11, -- link radius
-    DRAW_IN                = 12, -- 1 - player draw in, 2 - alliance draw in -- only add as a spawn mod!
+    DRAW_IN                = 12, -- bitmask with different binary options for draw-in (see DRAWIN enum for options)


To make this even more obvious (since this is a departure from the existing logic) I would probably name this DRAW_IN_BITMASK or DRAW_IN_MASK or DRAW_IN_SETTINGS etc.

zach2good · 2024-08-16T08:42:58Z

src/map/ai/controllers/mob_controller.cpp

@@ -710,11 +710,11 @@ void CMobController::Move()
        if (((currentDistance > closeDistance) || move) && PMob->PAI->CanFollowPath())
        {
            // TODO: can this be moved to scripts entirely?
-            if (PMob->getMobMod(MOBMOD_DRAW_IN) > 0)
+            if ((PMob->getMobMod(MOBMOD_DRAW_IN) & DRAWIN::DRAWIN_NORMAL) > 0)


The >0 is redundant here

zach2good · 2024-08-16T08:44:15Z

src/map/ai/controllers/mob_controller.cpp

+    // get specific draw-in settings that can be set on any given mob otherwise use default values
+    uint32 drawInTriggerDistance = PMob->getMobMod(MOBMOD_DRAW_IN_TRIGGER_DIST) > 0 ? PMob->getMobMod(MOBMOD_DRAW_IN_TRIGGER_DIST) : PMob->GetMeleeRange() * 2;
+    uint32 drawInMaxDistance     = PMob->getMobMod(MOBMOD_DRAW_IN_MAX_RANGE) > 0 ? PMob->getMobMod(MOBMOD_DRAW_IN_MAX_RANGE) : std::numeric_limits<int16>::max();
+    bool   includeAlliance       = (PMob->getMobMod(MOBMOD_DRAW_IN) & DRAWIN::DRAWIN_INCLUDE_ALLIANCE) > 0 ? true : false;
+    bool   toFrontOfMob          = (PMob->getMobMod(MOBMOD_DRAW_IN) & DRAWIN::DRAWIN_TO_FRONT_OF_MOB) > 0 ? true : false;


Same here, you don't need to be doing all of these extra steps...

const bool toFrontOfMob = PMob->getMobMod(MOBMOD_DRAW_IN) & DRAWIN::DRAWIN_TO_FRONT_OF_MOB;

Also, if you're not going to change the value of the variable later, make it const.

zach2good · 2024-08-16T08:44:49Z

src/map/entities/mobentity.h

+    DRAWIN_NONE             = 0x000, // no draw-in (mob can only draw-in by explictly calling the lua drawin function)
+    DRAWIN_NORMAL           = 0x001, // basic draw-in where mob will draw-in battle target when target some distance from mob
+    DRAWIN_TO_FRONT_OF_MOB  = 0x002, // determines if draw-in to the center of mob (bit not set) or front of mob (bit set)
+    DRAWIN_INCLUDE_ALLIANCE = 0x004  // determines if draw-in includes just the battle target (bit not set) or entire alliance (bit set)


Needs trailing comma (if that's relevant in C++, I don't remember?)

zach2good · 2024-08-16T08:45:59Z

src/map/utils/battleutils.cpp

-                    PMember->loc.p.z = nearEntity.z;
+                    if (toFrontOfMob)
+                    {
+                        PMember->loc.p.x = nearEntity.x;


Would it make sense to calculate these positions at the very start of the draw in, validate them on the navmesh, and then assign them here?

Would it make sense to calculate these positions at the very start of the draw in, validate them on the navmesh, and then assign them here?

I think the function is already doing what you describe? It calculates and validates near the start of the draw-in function. Or do you mean even earlier in the draw-in process like in a different function?

TracentEden force-pushed the improve_draw_in_specification_with_local_vars branch from 4a70fea to 35b8b95 Compare July 14, 2024 19:52

TracentEden2 changed the title ~~Improve draw-in specification with local vars~~ [core, lua] Improve draw-in specification with local vars Jul 14, 2024

TracentEden force-pushed the improve_draw_in_specification_with_local_vars branch from 35b8b95 to f307043 Compare July 26, 2024 22:46

TracentEden2 changed the title ~~[core, lua] Improve draw-in specification with local vars~~ [core, lua] Improve draw-in specification to fix some issues Jul 26, 2024

TracentEden force-pushed the improve_draw_in_specification_with_local_vars branch from f307043 to 832444e Compare July 26, 2024 23:11

TracentEden2 marked this pull request as ready for review July 26, 2024 23:11

TracentEden force-pushed the improve_draw_in_specification_with_local_vars branch from 832444e to c3d43eb Compare July 29, 2024 12:02

zach2good reviewed Aug 16, 2024

View reviewed changes

Improve drawin specification with more flexibility

696be33

TracentEden force-pushed the improve_draw_in_specification_with_local_vars branch from c3d43eb to 696be33 Compare September 5, 2024 00:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core, lua] Improve draw-in specification to fix some issues #6007

[core, lua] Improve draw-in specification to fix some issues #6007

TracentEden2 commented Jul 14, 2024 •

edited

Loading

Xaver-DaRed commented Jul 14, 2024 •

edited

Loading

TracentEden2 commented Jul 14, 2024 •

edited

Loading

Xaver-DaRed commented Jul 14, 2024 •

edited

Loading

TracentEden2 commented Jul 14, 2024

TracentEden2 commented Jul 26, 2024

WinterSolstice8 commented Jul 28, 2024

TracentEden commented Jul 29, 2024

zach2good Aug 16, 2024

zach2good Aug 16, 2024

zach2good Aug 16, 2024

zach2good Aug 16, 2024

zach2good Aug 16, 2024

zach2good Aug 16, 2024

TracentEden Aug 16, 2024

[core, lua] Improve draw-in specification to fix some issues #6007

Are you sure you want to change the base?

[core, lua] Improve draw-in specification to fix some issues #6007

Conversation

TracentEden2 commented Jul 14, 2024 • edited Loading

What does this pull request do?

Steps to test these changes

Xaver-DaRed commented Jul 14, 2024 • edited Loading

TracentEden2 commented Jul 14, 2024 • edited Loading

Xaver-DaRed commented Jul 14, 2024 • edited Loading

TracentEden2 commented Jul 14, 2024

TracentEden2 commented Jul 26, 2024

WinterSolstice8 commented Jul 28, 2024

TracentEden commented Jul 29, 2024

zach2good Aug 16, 2024

Choose a reason for hiding this comment

zach2good Aug 16, 2024

Choose a reason for hiding this comment

zach2good Aug 16, 2024

Choose a reason for hiding this comment

zach2good Aug 16, 2024

Choose a reason for hiding this comment

zach2good Aug 16, 2024

Choose a reason for hiding this comment

zach2good Aug 16, 2024

Choose a reason for hiding this comment

TracentEden Aug 16, 2024

Choose a reason for hiding this comment

TracentEden2 commented Jul 14, 2024 •

edited

Loading

Xaver-DaRed commented Jul 14, 2024 •

edited

Loading

TracentEden2 commented Jul 14, 2024 •

edited

Loading

Xaver-DaRed commented Jul 14, 2024 •

edited

Loading