Turret Mounting System V1.10

[Moderator]

14:07, 14th Apr 2013
Magtheridon96:

Approved. 5/5
This is a wonderful system and I am really proud of you Tankehpoo.
I bet you're really mad right now.

There are tiny pedantic improvements though.
In the TMS_Loop, you can use Get/SetWidgetLife instead of the Get/SetUnitState calls.
Your death check in TMS_Loop should use the IsUnitType and the GetUnitTypeId natives for more accurate results.
Yes, that last one is not very pedantic. You go ahead and update this, okay?
Please don't kill me, I have children to attend to ;_;

Old Review

Comments

• You can use elseif here:
  
  
  Code

  else
      if (TurretCount < 1) then
          call DisplayTextToPlayer(GetOwningPlayer(u), 0, 0, udg_TMS_ErrorMessages[2])
      else
          call DisplayTextToPlayer(GetOwningPlayer(u), 0, 0, udg_TMS_ErrorMessages[3])
      endif
  endif

• The entire TMS_Loop function can be improved if you were to rely on an if/elseif/elseif/elseif/else structure.
  This is because all of the if blocks nested into each other are evaluating mutually exclusive conditions (since they are reading the same variable)
  
• In case you didn't know this, you can pass 0 without a decimal point as a real and it would be totally fine in JASS
• Use GetWidgetLife and SetWidgetLife instead of those SetUnitState calls. (Only for current life)
• Instead of WEAPON_TYPE_WHOKNOWS, you can use null because they're pretty much the same thing.
• Make the ATTACK_TYPE and DAMAGE_TYPE configurable D:
• You may need to do a death check like so: not IsUnitType(u, UNIT_TYPE_DEAD). Beware though, this returns true for null units D:
  In addition to this, you can add a life check. You shouldn't compare with 0 however, you should compare with 0.405
  You can never be 100% accurate in such a case. Reincarnating units need to be taken into account. Another solution is to add a GetUnitTypeId(...) != 0 check. You'd have to experiment with all these to get the best results
• Code

  if ((udg_TMS_TargetStatus[udg_TMS_TurretType[udg_TMS_Index]] == 5) or (udg_TMS_TargetStatus[udg_TMS_TurretType[udg_TMS_Index]] == 6)) and ((IsUnitType(u, UNIT_TYPE_GROUND)) and (not IsUnitType(u, UNIT_TYPE_STRUCTURE))) then
      call GroupRemoveUnit(udg_TMS_TempGroup, u)
  elseif (IsUnitType(u, UNIT_TYPE_GROUND)) then
      call GroupRemoveUnit(udg_TMS_TempGroup, u)
  endif

  IsUnitType(u, UNIT_TYPE_GROUND) can be taken outside of both of those expressions and put in the if block that surrounds this snippet of code.
  You would then change the logic in both of the conditional expressions above making them assume that the unit is in fact a ground unit because the outer if block would have assured that
• Location leak

  set udg_TMS_TempPoint2 = Location(GetUnitX(udg_TMS_UnitIndex[udg_TMS_SecondaryIndex]), GetUnitY(udg_TMS_UnitIndex[udg_TMS_SecondaryIndex]))
  set udg_TMS_TempReal2 = GetLocationZ(udg_TMS_TempPoint2)

  This location should be removed directly after you get the Z of the location.
• Wanna know an alternative solution to something like this?
  
  
  Code

  set my_effect = AddSpecialEffect(...)
  call DestroyEffect(my_effect)

  
  It looks a little bit like this:
  
  
  Code

  call DestroyEffect(AddSpecialEffect(...))

  
  This is applicable to a lot of sections in your code.
• Code

  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 1) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_MAX_LIFE)
      if (udg_TMS_TempReal <= udg_TMS_TempReal2) then
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      else
          call TMS_FilterData(u)
      endif
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 2) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_LIFE)
      if (udg_TMS_TempReal <= udg_TMS_TempReal2) then
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      else
          call TMS_FilterData(u)
      endif                
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 3) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_MAX_LIFE)
      if (udg_TMS_TempReal < udg_TMS_TempReal2) or (udg_TMS_TempReal2 == -1.00) then
          call TMS_FilterData(u)
      else
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      endif
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 4) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_LIFE)
      if (udg_TMS_TempReal < udg_TMS_TempReal2) or (udg_TMS_TempReal2 == -1.00) then
          call TMS_FilterData(u)
      else
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      endif   
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 5) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_MAX_MANA)
      if (udg_TMS_TempReal <= udg_TMS_TempReal2) then
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      else
          call TMS_FilterData(u)
      endif
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 6) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_MANA)
      if (udg_TMS_TempReal <= udg_TMS_TempReal2) then
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      else
          call TMS_FilterData(u)
      endif
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 7) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_MAX_MANA)
      if (udg_TMS_TempReal < udg_TMS_TempReal2) or (udg_TMS_TempReal2 == -1.00) then
          call TMS_FilterData(u)
      else
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      endif
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 8) then
      set udg_TMS_TempReal = GetUnitState(u, UNIT_STATE_MANA)
      if (udg_TMS_TempReal < udg_TMS_TempReal2) or (udg_TMS_TempReal2 == -1.00) then
          call TMS_FilterData(u)
      else
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      endif
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 9) then
      set udg_TMS_TempX = GetUnitX(udg_TMS_MountedUnit[udg_TMS_Index])
      set udg_TMS_TempY = GetUnitY(udg_TMS_MountedUnit[udg_TMS_Index])
      set udg_TMS_TempX2 = GetUnitX(u)
      set udg_TMS_TempY2 = GetUnitY(u)
      set udg_TMS_TempReal = ((udg_TMS_TempX2 - udg_TMS_TempX) * (udg_TMS_TempX2 - udg_TMS_TempX)) + ((udg_TMS_TempY2 - udg_TMS_TempY) * (udg_TMS_TempY2 - udg_TMS_TempY))
      if (udg_TMS_TempReal < udg_TMS_TempReal2) or (udg_TMS_TempReal2 == -1.00) then
          call TMS_FilterData(u)
      else
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      endif            
  endif
  if (udg_TMS_TargettingMethod[udg_TMS_TurretType[udg_TMS_Index]] == 10) then
      set udg_TMS_TempX = GetUnitX(udg_TMS_MountedUnit[udg_TMS_Index])
      set udg_TMS_TempY = GetUnitY(udg_TMS_MountedUnit[udg_TMS_Index])
      set udg_TMS_TempX2 = GetUnitX(u)
      set udg_TMS_TempY2 = GetUnitY(u)
      set udg_TMS_TempReal = ((udg_TMS_TempX2 - udg_TMS_TempX) * (udg_TMS_TempX2 - udg_TMS_TempX)) + ((udg_TMS_TempY2 - udg_TMS_TempY) * (udg_TMS_TempY2 - udg_TMS_TempY))
      if (udg_TMS_TempReal <= udg_TMS_TempReal2) then
          call GroupRemoveUnit(udg_TMS_TempGroup, u)
      else
          call TMS_FilterData(u)
      endif            
  endif

  This entire code block can rely on the elseif keyword since all the conditional expressions are mutually exclusive. You are evaluating the same variable value. No two if blocks will run at once, so why check all of them? . Using the elseif keyword will assure that you will stop checking the rest of the conditional expressions as soon as the correct one has been found.
• Code

  set udg_TMS_TempReal3 = SquareRoot(udg_TMS_TempReal2 * udg_TMS_TempReal2 + udg_TMS_TempReal * udg_TMS_TempReal)
  if (udg_TMS_SelectedTarget[udg_TMS_Index] == null) or (not udg_TMS_TargetLock[udg_TMS_TurretType[udg_TMS_Index]]) or (udg_TMS_TempReal3 > udg_TMS_Range[udg_TMS_TurretType[udg_TMS_Index]]) then

  In this snippet of code, you can get rid of the SquareRoot call and just compare TempReal3 with udg_TMS_Range*udg_TMS_Range.
• You're sometimes restarting the system timer when you don't need to. With the low timeout that the system has right now, this is totally fine, but with larger timeouts and heavy turret adding, the effects would be terrible . You should only start the timer if the instance that you're registering happens to be the first instance in the list, meaning the total turrent count is now exactly 1. (after you increment it while taking account the current instance of course)

Other than that, this looks like a lovely system and I'd be more than happy to give it a high rating. It's not the most efficient beast out there, but it's wicked sick either way!

 

[Tank-Commander]

If anybody would be able/willing to create a dedicated model for this system for mounting turrets onto while looking aesthetically pleasing that would really be the icing on the cake.

if somebody does, (or knows a good model for this) link it to me in PM or a post, and I'll add it to the description

~reserved

 

[Almia]

Woah!
Tank- Commander released a system yay! and it features tanks too :3

Uhmm,to reduce variables,i think you should use constants.But meh,it will make the script longer.

You should use hashtable and an encapsulation method.Register the available turrets and its data to that function then save it for every instance the Register is called.
Example:

function Register takes integer id, string mdl returns nothing
    call SaveString(udg_Hash, id, 0, mdl)
endfunction

function Init takes nothing returns nothing
    call Register('hfoo', "Hello")
endfunction

Also,let them register the turret and datas they want. Just put the datas on demos.

About the projectile effects,you don't need them to add their functions.Let them register the effects they want via either code or trigger.

                    set udg_TMS_TempReal2 = udg_TMS_TempReal
                    if (not(udg_TMS_TempUnit3 == null)) then
                        call GroupRemoveUnit(udg_TMS_TempGroup, udg_TMS_TempUnit3)
                    endif
                    set udg_TMS_TempUnit3 = udg_TMS_TempUnit2

Cache this into a function.

You should also read this mini-tutorial : http://www.hiveworkshop.com/forums/tutorial-submission-283/use-functions-other-triggers-232537/
That tutorial teaches you not to use Map Header for your functions. And also this works 100% not to encounter any code errors.

if (udg_TMS_TempBoolean == false) then
->
if not udg_TMS_TempBoolean then

    call UnitAddAbility(udg_TMS_UnitIndex[udg_TMS_MaxIndex], 'Amrf')
    call UnitRemoveAbility(udg_TMS_UnitIndex[udg_TMS_MaxIndex], 'Amrf')

->

 if UnitAddAbility(udg_TMS_UnitIndex[udg_TMS_MaxIndex], 'Amrf') and UnitRemoveAbility(udg_TMS_UnitIndex[udg_TMS_MaxIndex], 'Amrf') then
endif

For safety.

set udg_TMS_Angle = GetRandomReal(0, 360) * bj_DEGTORAD
->
set udg_TMS_Angle = GetRandomReal(-bj_PI, bj_PI)

set udg_TMS_TimerSpeed = 0.03125
Add "0000" at the end of 5 ( reals are inaccurate )

RemoveUnit function does not guarantee you to clean leaks.
The guaranteed one is by setting the Death Animation to 0 and Fix the death type of the dummy (It should be Can't raise, does not decay).

 

[Maker]

RemoveUnit function does not guarantee you to clean leaks.
The guaranteed one is by setting the Death Animation to 0 and Fix the death type of the dummy (It should be Can't raise, does not decay).

Remove unit is enough to remove the unit correctly.

Units always leak.

 

[Tank-Commander]

Modifications in progress, also updating Readme, will edit this post when updates are completed.

Edit: Initial Modifications complete

Notes: Almia, explain why the remove/add abilities are in an if statement/why that works? if you be so kind, logically doesn't make much sense to me
additionally, I feel adding 4 "0"s on the end of "0.03125" is fairly superfluous but if you think it'd be benefitial to the system then I can't see a reason why not

 

[Almia]

It detects whether the unit already has the 'Amrf'(Crow form), much safetier and faster because ifs ignores other functions.

 

[Maker]

You can test the real inaccuracy with this I think.

library TimerTest initializer init
    globals
        private timer t1 = CreateTimer()
        private timer t2 = CreateTimer()
        private timer t3 = CreateTimer()
        private constant real to1 = 0.001
        private constant real to2 = 0.0010000
        private constant real addA = 0.001
        private constant real addB = 0.0010000
        private real totalA = 0
        private real totalB = 0
        private boolean pause
    endglobals
    
    private function results takes nothing returns nothing
        call DisplayTimedTextToPlayer(Player(0), 0, 0, 10, "Timer A: " + R2S(totalA))
        call DisplayTimedTextToPlayer(Player(0), 0, 0, 10, "Timer B: " + R2S(totalB))
    endfunction
    
    private function timerEndB takes nothing returns nothing
        set totalB = totalB + addB
        if pause then
            call PauseTimer(t2)
        endif
    endfunction
    
    private function timerEndA takes nothing returns nothing
        set totalA = totalA + addA
        if pause then
            call PauseTimer(t1)
            call TimerStart(t3, 0.05, false, function results)
        endif
    endfunction
    
    private function StartTimers takes nothing returns nothing
        if ModuloInteger(GetTriggerExecCount(GetTriggeringTrigger()), 2) == 1 then
            call DisplayTimedTextToPlayer(Player(0), 0, 0, 10, "Start")
            set pause = false
            call TimerStart(t1, to1, true, function timerEndA)
            call TimerStart(t2, to2, true, function timerEndB)
        else
            set pause = true
        endif
    endfunction

    private function init takes nothing returns nothing
        local trigger t = CreateTrigger()
        call TriggerRegisterPlayerEvent(t, Player(0), EVENT_PLAYER_END_CINEMATIC)
        call TriggerAddAction(t, function StartTimers)
    endfunction
    
endlibrary

I let it run for 73 seconds and both values were the same, though the message only displays three decimals.

 

[Almia]

Idk how Mag exceeded the 3 decimals in the text.

edit
The DEGTORAD at GetRandomReal(-bj_PI,bj_PI) * bj_DEGTORAD must be removed. It is already in radians.

GetOwningPlayer(u) must be cached.

You can replace this:

function InitTrig_TMS_TurretMountingSystem takes nothing returns nothing
    //Error messages for the system
    set udg_TMS_ErrorMessages[0] = "This unit cannot have turrets mounted"
    set udg_TMS_ErrorMessages[1] = "Turret limit reached"
    set udg_TMS_ErrorMessages[2] = "You need to specify the number of turrets to mount"
    set udg_TMS_ErrorMessages[3] = "You need to specify the turret type number"
    //Determines the default limit for units not registered in the Carrier Units setup
    set udg_TMS_DefaultLimit = 0
    // Determines the how long death effects play for before the unit is removed
    set udg_TMS_DeathTimer = 1.00
    // Determines the dummy used for the turrets
    set udg_TMS_DummyType = 'u001'
    // --------
    // Determines the Speed of the loop trigger
    set udg_TMS_TimerSpeed = 0.031250000
endfunction

With constant functions.

If you are going to use some TempVariables for just one function,just use locals instead(to reduce mass variable importing.JASS can't support instant variable CnP in preferences > Create unknown variables while pasting data)

 

[Bloody Wolf]

so this is what you were talking about on the chat..i must say you did a great job with the turrets and off course stunned everione again XD 5\5 but like you said you have to watch out for the lag

 

[gorillabull]

lol the code looks like converted gui at least use a few locals and globals, it takes too long to write udg_ before each variable >__>
but its good to see that you finally started using jass

 

[Tank-Commander]

It's written in pure JASS, to my understanding, that makes "globals" blocks not useable, so the configuration, would still be using udg_ all the time, you can just CnP the config and that saves you the effort of typing any of it but the values you want, as for locals, I already do in quite a few places, though I suppose I could use more, possibly.

Currently working on a physics improvement

 

[Bloody Wolf]

just tested it and holy **** man you are a freakin genius

 

[Auildere]

What units needs to be used to allow to mount the tank?

 

[Tank-Commander]

If you look at the readme, it tells you how to register new carrier units and how much turret weight they can carry, you don't need to use a particular base unit, to make carriers.

 

[Auildere]

oh thanks.

 

[Auildere]

i dun understand the instructions cuz i'm a noob.

 

[Tank-Commander]

well I cannot explain more clearly than I already have, I'd just repeat myself as I wrote in the readme

 

[Auildere]

i just don't know much of triggers yet

 

[Tank-Commander]

yeah, I'm aware of the issue, however I can't do much to solve it because if I set it to "true" physics, then some shots the turret AI will take, cannot hit, and the processing would increase tenfold to make the AI account for such when selecting targets as to make it only select targets it can hit, and the system already uses a lot of processing, so you're a bit stuck with the slightly wobbly arcs with sea-to-land battles I'm afraid.

 

[Auildere]

i don't quite understand how to have the model set up.....

 

[mckill2009]

I really do hope you'll make a vjass version...

 

[InfernalTater]

Of course you did, Tank-Commander.

5/5.

 

[Strikest]

This is cool but it cause leak. Cuz when I tested it in the map, i let the tanks attack a knight for like 10 minutes. Then after, when I quit map, I got black screen for like 5 min(Warcraft 3 not responding, couldn't use task manager to End Task).

 

[Tank-Commander]

You know, that doesn't necessarily mean there's a memory leak (or leaks), if there is one you can find, I will of course remove it, but a non-specific speculative reasoning isn't exactly going to help with that - since it gives no indication of where to even begin looking. After all it could've been a lack of RAM in your Computer system or just the game failing (along with a few other explanations)

 

[Ardenaso]

so it's like those airships in Unmodern Warfare that can carry gunners that can also shoot while loaded?

 

[Barade]

Hi, this is a nice system and really useful for Modern Warfare maps but I am wondering about:

• Why no manual targeting example in the demo map or did I miss it? You could simply catch smart orders of the tank and translate them into attack orders/targeting from the turret. The tank only has to move in range and the target should have priority then. Only auto targeting in the demo map makes it hard to image the system if you still want to have the control over the targets. The big advantage of the turrets in my opinion is that you can move while shooting (like Phoenix Fire) but the targeting would be something which Phoenix Fire does not have.
• How would AI handle tanks with turrets? Since the tanks have no attack, do they just move around with the army randomly?
• When the turret kills a unit, is the tank the killing unit or the turret? I guess the turret right?
• It seems that you create the missiles manually with code from the turrets and the turret models as well? Why not create dummies as turrets which are clickable of configured and can have their own HP and be healed/repaired?
• The tanks in the demo map seem to flee when hit which is a bit annoying.
• Could you show the damage as regular damage for the tank unit BUT catch attack orders, so the tank will never really attack but the turret will focus the target instead if it is range.
• It seems that all turrets from one carrier fire at the same target? Is it possible to allow them attacking different targets?

I think it is a great useful system for modern warfare maps BUT I think it would be much better to create dummies for the different turret types with Warcraft's attack/missile engine. You can still use the custom auto targeting code to prefer certain targets but the whole code for projectiles will be obsolete which would probably improve the performance a lot! You just have to auto update the positions of the turret units and their targets. The code for the loop TMS_Loop is pretty long. I am not even sure if you want to specify specific targets if you use Warcraft dummy units with their own attacks. If not, the loop code would only update the turret positions and issue them to attack a focused target if it is in range which would really reduce the code.
You would not need the moving special effects either with one dummy unit type per turret type since you only have to move the dummy.

edit:
I tried it myself and if you use dummy units for turrets you have to auto move them with SetUnitX and SetUnitY as described here: Attacking WHILE moving!
Otherwise, the attack gets interrupted.