Could use some help

[freewilly]

I'm still poking around with NewPull.mac. It's what I use to pull with and it's what I am cutting my teeth on learning how to write these macros. I've spent the last two days, literally almost 48 hours, trying to work out my sub Combat but while I am getting close to something working consistently, it still acts wonky every now and then:

:CombatLoop
    /if (${Target.ID} && ${Target.Type.NotEqual[corpse]}) {
	/killthis
	/popup FIRST!
	/if (!${Target.ID} && !${Me.XTarget}) /return
    /if (${Me.CombatState.Equal[COMBAT]}) /goto :CombatLoop
	}
	/delay 8
	/target clear
	/if (${Me.XTarget}) {
	/beep
	/squelch /tar ${Me.XTarget[1]}
	|/if (!${Target.LineOfSight}) /face fast
	/face fast
	/if (${Me.XTarget}) /goto :CombatLoop
	  }
/return

Sub Combat is called from Main sub after the pull sub is complete and character is back at camp. The first if statement in :CombatLoop checks to make sure I have a target and it's not a corpse ( at least that is what I think it is doing). It then used /killthis to attack the mob, the popup statemetn is just to let me know where in the loop/macro things are at. The second if statement is to give the loop a way to break and return to sub Main (not sure I need it). The third if statement just loops me back to :CombatLoop as long as I am still in combat mode. This loop seems to work pretty well.

Where I have an issue is if there are adds. I want to kill all adds before returning to sub Main and looting, but sometimes he gets stuck targeting the first kills corpse, sometimes he doesn't. It's like he can't break out of :CombatLoop even though I think I have the necessary checks in place to break off the corpse and engage the add(s).

I kind of look at Sub Combat as two parts, :CombatLoop to handle the fighting part, and starting with the 4th if statement being able to handle any adds and call back to :CombatLoop. The delay and /target clear were attempts to get him off the corpse. The delay seemed to help with my issue, and in fact I may just need to increase that to fix it completely. The /target clear just does not work and needs to be removed. If you note the /beep after the 4th if statement, again that was a trigger I used to see where the macro was at. What I found before I put the delay in was that even with single pulls I was getting beeps, which by my thinking should not be occurring since there was nothing in XTarget1. SO it seemed that the macro was just going too fast and was shooting through the :CombatLoop. Like I said, the delay improved this issue (I even set Turbo to 80 to try and slow things down).

So, my question is, does this piece of code look ok to those who know better than me (everyone), and I just need to increase the delay, or are their flaws in the logic I used for the loop?

 

[Xeniaz]

/if (!${Target.ID} && !${Me.XTarget}) /return
:CombatLoop
  /if (${Target.ID} && ${Target.Type.NotEqual[corpse]} && ${Target.Distance} < 50) {
    /killthis
    /popup FIRST!
	}
	/if (${Me.CombatState.Equal[COMBAT]}) /goto :CombatLoop
	/delay 8
	/target clear
	/if (${Me.XTarget}) {
  	/beep
  	/squelch /target ${Me.XTarget[1]}
  	| /if (!${Target.LineOfSight}) /face fast
  	/face fast
  	/if (${Target.ID}) /goto :CombatLoop
	}
/return

I changed some of your logic. First, I moved the check for not having a target, or xtarget outside of the combatloop. That is so it's not getting checked each round through that loop (quite often). Second, I changed your second if statement to also have a range check. You may have aggro on something, but it might not be anywhere near camp. (perhaps someone stole it, or the mob got lost). Next up is your line that checks if you are in combat, I moved it to be outside of the second if statement's body, it isn't a good fit there. (Note, you should add variables to keep track of the current mob's ID, rather than checking the combat state. You will still be in combat if you have adds.) In your last if statement, you were checking if you had an xtarget, I changed that to check if you had a target, since that code is inside of a check for xtarget, you instead want to know that the code actually found a target for you. Feel free to ask questions!

 

[Cr4zyb4rd]

(again beaten to the punch, oh well now you have two answers)

As a first blush, let's clean up the formatting just a little bit. By simply removing all of the pseudo-random tabs and reformatting, we arrive at this:

:CombatLoop
	/if (${Target.ID} && ${Target.Type.NotEqual[corpse]}) {
		/killthis
		/popup FIRST!
		/if (!${Target.ID} && !${Me.XTarget}) /return
		/if (${Me.CombatState.Equal[COMBAT]}) /goto :CombatLoop
	}
	/delay 8
	/target clear
	/if (${Me.XTarget}) {
		/beep
		/squelch /tar ${Me.XTarget[1]}
		|/if (!${Target.LineOfSight}) /face fast
		/face fast
		/if (${Me.XTarget}) /goto :CombatLoop
	}
/return

Now that the flow/logic is (hopefully) a bit more transparent and human-readable, let's take it step by step and see how it jives up with what you described.

First we check that are target is valid. We then /killthis to turn on mq2melee and immediately check validity of our target, something that's not likely to have changed since one command ago... since this appears to be the "exit condition" of the loop, perhaps it belongs somewhere else?

We then have a delay. Not inside of a condition, not as an action to take after some other action, but just a bare delay. This could be what we want, but why here? All we've done is possibly issue one command, a toggle for further automation handled by an external plugin.

Having paused briefly, we clear the target.

In this next bit, we check for an XTarget, in which case we target it, face it, make that exact same check again and restart the loop if it passes (it will, it's the exact same check...unless we're so badass we kill things just by looking at them.)

The logic's not too far skewed, but let's see if we can do the same thing a little more cleanly. Here's what I came up with.

|Loop until we no longer have an XTarget
:CombatLoop
	/if (!${Target.ID} || ${Target.Type.Equal[corpse]}) {
		/squelch /target ${Me.XTarget[1]}
		/delay 2s ${Target.ID}
		/face fast
	}
	/if (!${Me.CombatState.Equal[COMBAT]}) /killthis
	/delay 3
	/if (${Me.XTarget}) /goto :CombatLoop
/return

First thing last: As long as we have an XTarget, we want to continue looping, so we'll make that if the very last line of our sub. Establishing this outer/boundary condition is important to how we think about our loop or subroutine as a whole. We'll insert a comment to commemorate our victory and serve as a landmark for the next guy (possibly an older us) that ventures here.

Now let's look at making the loop actually do what we want...

Do we have no target or an invalid target? Let's fix that, and add a short delay that ends early as soon as we have a valid target.

We have a target now.. are we fighting it? If not, let's do so.

A short delay just to play nice, and we're at our bounds-check.

 

[freewilly]

Thank you

Thank you both very much for the help.

 

[freewilly]

In case anyone is interested, I went a different kind of way with this combat sub. I broke the loop out of Combat sub and made another sub with :combatloop in it. This appears to be working as I intended. I also added to the macro the ability to pull with a bow, so the combat sub has gotten a little bigger. This is probably cludgey to those Pro's here, but I am learning. Thanks again to Xeniaz and Cr4zyB4yd.

Sub CombatSub
| Loading MQ2MoveUtils so that sticking is handled by MQ2Melee.  It will be unloaded in Loot sub.
    /if (!${Plugin[MQ2MoveUtils].Name.Equal[MQ2MoveUtils]}) /squelch /plugin MQ2MoveUtils load
	/if (!${Target.ID} && !${Me.XTarget}) /return	 
	/if (${Me.XTarget}) {
	/target ${Me.XTarget[1]}
	/delay 3
	/bandolier activate Tank
       /call CombatLoop
       /delay 1s	
  }
       /if (${Me.XTarget}) {
       /squelch /target ${Me.XTarget[1]}
      /call CombatLoop
  }
/return
     	 
|--------------------------------------------------------------------------------
|SUB: CombatLoop
|--------------------------------------------------------------------------------
Sub CombatLoop
     :CombatLoop
	 /killthis
	 /if (!${Target.ID} && !${Me.XTarget}) /return
	 /if (${Me.XTarget} && ${Target.Type.NotEqual[corpse]}) /goto   :CombatLoop
/return