Zarel uploaded file smarterstructures.patch (7.0 KiB)

per commented

This patch touches some rather delicate parts of the code, and needs careful review. Some general comments:

Please don't leave your nickname in comments. Looks cute right now, but gets really tiring later on. If someone wants to know who did something, there is always svn blame.

// <<3 is faster than *9

Not sure what the above comment refers to, since there is no <<3 or *9 anywhere. In any case, this would be a very premature optimization.

Don't add commented out code.

You should be wary of changing NAYBOR_RANGE, since it has a huge performance impact. Also the extra loop over all buildings in visibility.c makes visibleObject() a lot slower, and this should be considered carefully.

Most of these fixes should be discussed on their own. For example, making a sensor turret turn toward its target instead of rotating is not even in the patch description!

Zarel commented

Sorry about leaving my nickname over the code. It makes it easier to search for my changes when something goes wrong on my local copy, and I saw Watermelon's name in some places, so I didn't really think to take them out when I made the patchfile. I won't leave it in next time.

The "<<3 is faster than *9" comment is in reference to the "*8", which most compilers should interpret as "<<3".

I'll change NAYBOR_RANGE back and take out the sensor turning - it was mostly useful for debugging. I didn't check to see how many places visibleObject is called, so I'll think of a faster way to implement that check.

Per changed status from new to accepted

Per commented

The sensor turning is quite interesting, actually. Although, if added it should be done properly, not just warped to the right direction - but smoothly turned just like weapon turrets.

We should discuss how to improve the naybor code. It is not good. But I think that is a separate and more general issue.

Per changed owner from `` to Per

Per commented

Sorry, I misread your patch. It already does the right thing in regards to sensor rotation. I tested it a bit, and based on that, I would suggest that it rotates toward targetted droids only.

Zarel commented

Whoa, whoa, whoa. "Accepted"? I was planning to resubmit this as a bunch of smaller patches.

Per commented

I think that just means I'm assigning it to myself for follow-up... But then I'm new to this Trac thing.

Zarel uploaded file evensmarterstructures.patch (7.8 KiB)

Here's part one of the many parts I split my patch into.

Zarel commented

evensmarterstructures.patch implements and fixes:

• Structures no longer target things they can't shoot because of terrain blocking. (This fixes the situation where a structure is targeting a nearby enemy droid it cannot attack, while other faraway but still within range droids are attacking it)

• Structures will target trucks before unfinished structures, if possible.

• Structures will attack walls if there are enemy droids/structures behind them.

• Fortresses and a few other towers are no longer considered bunkers.

Per commented

I don't quite get the changes to structures.txt. What are you doing to WreckedTransporter?

I will try to test it this weekend.

Zarel commented

The changes to structures.txt are to make structures that aren't as small as bunkers not be treated as small as bunkers (as in, low enough that it can't fire over a wall).

The game treats any structure with height 1 as "not tall enough to fire over a wall". So I changed all towers to height 2 (most already were), and fortresses to height 3. I just changed WreckedTransporter to height 2, as well, so any code meant for structures that small won't affect it.

Per commented

First patch of the patch committed. Can you explain what the "radSquared = 4*radSquared;" part of the patch does? This should have been commented in the code, since it looks rather odd. Besides, it is missing curly braces.

Per commented

I do not like the changes to visibility.c. visibleObject() is meant to check whether object A can see object B, and is used to determine general visibility. You cannot therefore use general visibility in visibleObject() to determine whether object A sees object B - first, you'd lose the purpose of the function, and second, there is a bad circular dependency here.

anonymous commented

I've commented the 4*radSquared part and added braces. It's intended so structures can fire on things it can see out to twice its sensor range.

I didn't see the code that uses visibleObject to determine general visibility earlier. The best solution, I think, is to add another argument to the function. Are you okay with that?

Per commented

Why twice its sensor range? It seems like a completely arbitrary rule. (I am sure you meant to write "so structures can fire on things the player can see, out to twice its sensor range.")

I am not sure I like adding an argument to visibleobject, either. Such logic should be added where visibleObject() is called, not inside it.

Also something that looks strange is "tarDist = proj_GetLongRange(psWStats)*proj_GetLongRange(psWStats);", because surely the earlier comparison and the assignment of this value should be same? What is wrong with distSq in this case?

Per commented

Also in ai.c, there should be a check with aiStructHasRange() in there. Direct fire structures sometimes assign targets to themselves that they can't (ever) hit, because weapons range is lower than sensor range. All the other checks in that function checks for this, except the one being changed.

Zarel commented

Twice the sensor range is enough so that range 2048 weapons will always be able to fire their range, but it isn't something ridiculous like 10x the sensor range, which might cause a lot of slowdown. At twice the sensor range, the only things that won't be able to fire their full range are Missile Fortress and SAM tanks before Sensor Upgrade. Therefore, I think twice sensor range is a good compromise between speed and longrange.

distSq is the sensor range squared. proj_GetLongRange, etc is the weapon range squared. My newer code sets tarDist to weapon range squared by default (instead of UDWORD_MAX or whatever it was), so that line can be taken out. This also negates the need for aiStructHasRange.

How would you add the logic where visibleObject is called? Adding an argument seems to be the best/fastest way to do it.

I didn't have code for aiStructHasRange earlier, since I thought it was intentional that structures would turn to face whatever was assigned, so it could fire faster. After all, they'll only assign themselves to targets they can't hit if there's nothing within weapons range (or behind a wall, but my patch fixes that). The removal of that functionality was mainly for speed reasons.

Zarel commented

Do we agree yet?

Zarel uploaded file smarterstructures3.patch (8.2 KiB)

Here's the newest and best version.

Giel edited the issue description

Per uploaded file smarterstructures4-per.patch (8.6 KiB)

per's version

Per commented

Wondered if there was not a better way, so I gave it a shot myself. Be warned that the above patch is barely tested at all, but IMHO the design is better. It creates a new function lineOfFire() that is used in place of visibleObject() for determining who can fire where with direct weapons. Also changed structure targeting not to consider sensor but weapons range, and some improvements there generally.

Zarel commented

Although your patch does some things better, considering lineOfFire is nearly identical to visibleObject, I think it would be better implemented as an additional argument to visibleObject than a separate function.

Additionally, aside from changing the direct-fire targeting, your patch implements none of the bugfixes my patch contains; namely, it implements none of the following structure AI improvements mine implements:

• Structures no longer target things they can't shoot because of terrain blocking. (This fixes the situation where a structure is targeting a nearby enemy droid it cannot attack, while other faraway but still within range droids are attacking it)

• Structures will target trucks before unfinished structures, if possible.

• Structures will attack walls if they are blocking line-of-sight to enemy droids/structures behind them.

anonymous commented

Structures will target anything else before unfinished structures, including trucks. Calls to visibleObject()/lineOfFire() in the targeting code were deliberately not included this time around because they would perform a raytracing for each and every considered object, and that could be a lot. Raytracing is expensive, so this should be a separate patch that is tested for performance regressions. Since checking for direct line of fire is rather different from checking line of sight, these two functions may well develop in quite different directions in the future. I saw much potential for development but wanted the patch to remain small. - Per

Zarel commented

Oh, I didn't see how you implemented the

|| (psTarget && psTarget->type == OBJ_STRUCTURE && ((STRUCTURE *)psTarget)->status != SS_BUILT)

part. That's actually a pretty nice solution.

As for the raytracing - If we only implement raytracing only when the weapon is fully loaded, it will only call lineOfFire a number of extra times equal to the number of objects it would have originally targeted but cannot shoot. Since this would mean that the code will no longer continuously call lineOfFire (once per tick instead of once per reload) while a structure is targeting an object it cannot shoot, this would actually significantly IMPROVE performance, not make it worse, since it would call lineOfFire two or three times every firePause ticks (note that firePause is usually at least 30) instead of once every tick - AND the weapon would actually fire, finally fixing that particular targeting bug.

That said, I fully support your patch.

Per uploaded file smarterstructures5-per.patch (9.4 KiB)

I have committed several parts of the patches above now, and this is a new version of the targeting changes. It also changes droid targeting now.

Buginator commented

In [6183]
in ai.c you got:

WEAPON_STATS	*psWStats;
int             longRange = proj_GetLongRange(psWStats);

That can't be correct. I am guessing it should be something like:

WEAPON_STATS	*psWStats =((STRUCTURE*)psObj)->pStructureType->psWeapStat;
int             longRange = proj_GetLongRange(psWStats);

?

Buginator commented

Replying to Warzone2100/old-trac-import#97 (comment:22):

In [6183]
in ai.c you got:

WEAPON_STATS	*psWStats;
int             longRange = proj_GetLongRange(psWStats);

That can't be correct. I am guessing it should be something like:

WEAPON_STATS	*psWStats =((STRUCTURE *)psObj)->asWeaps[weapon_slot].nStat + asWeaponStats;
int             longRange = proj_GetLongRange(psWStats);

?

Buginator commented

Bah, I meant it was fixed in [6193].

anonymous commented

Gah. Copy & paste bug right before committing it. Sorry!

Buginator commented

Where are we at with the rest of this?

Per commented

All my fixes are now committed, and I do not intend to go further with this. Feel free to create further improvements on top of them.

Per changed blocking which not transferred by tractive

Per changed blockedby which not transferred by tractive

Per changed status from accepted to closed

Per set resolution to fixed

Buginator removed milestone (was 2.1)

Buginator commented

Milestone 2.1 deleted