- User Since
- Dec 21 2016, 1:38 PM (130 w, 6 d)
(to not forget)
- this reverted rP20053 by accident, and we should again disable the login/registration button when clicking on them to prevent acting twice, which crashes.
See inline for where I think we need to fix this.
I don't think there is much value in having both functions, but renaming to AppendPathnames makes sense.
Rebased - needs some cleaning, like the anti-cavalry formation no longer has a walk animation which is annoying - I guess I should group the animations better. The limit of one variant per file is quite annoying because it means one must change a lot of files every time -_-
I guess you don't understand the concept of an iteration or an MVP.
Reworked slightly with better comments and no navcell fix which should not be needed. I believe the GOAL_DELTA was intended to do something slightly similar at some point, and is also removed.
Actually working correctly this time.
@Itms Can you close a revision or should I abandon this?
@Freagarach feel free to commandeer this with the fixes above.
There is a simple, infaillible logic to making the hierarchical pathfinder not a sub-class of the long-range pathfinder: you can perfectly use the hierarchical pathfinder without a long-range pathfinder (the reverse isn't true).
If anything, we could make the hierarchical pathfinder own the long-range pathfinder, but the opposite didn't make sense.
I also made three of the patch's iterations. But it doesn't matter much.
The reason why I moved the stateful calls outside the functions was mostly in planning of further diffs, which remove StopMoving() and "FacePointAfterMove", i.e. the duplication.
True about the warning. I was merely following the spec at https://github.com/0ad/0ad/blob/master/source/simulation2/Simulation2.h#L193
As noted on IRC, this triggers an infinite loop in JS (so contrary to what said above, the segfault is related), because of some unknown (so far) issue with attacking as formation (which was removed by D1220 otherwise).
Do what I say above.
Sat, Jun 22
Looking much better like that, I think.
Fri, Jun 21
Do we really need that behaviour for wrong strings?
I don't think we do. Doubt we have any code in 0 A.D. that uses it, and I also doubt that any mod relies on that, because it's hardly useful anyhow.
Then: not deleting RecomputeBlurredNormalMap was an oversight, and it should be deleted.
It's not. Disabling reflections has never meant "show nothing". In fact, from the beginning reflections have been about entity reflection, so I guess your comment is correct. Again, I question your use of the concern option over a trac ticket.
I would certainly hope none otherwise MacOS has been behaving wrong for years.
The only place I can think of that parses strings to number is GUI sizes, which would previously parse things like "3b" as "0" instead of the 'correct' "3". I don't think there's mods or 0 A.D. code that relied on that - and I'm not sure we want that code to live.
I don't understand what you want me to change.
Thu, Jun 20
I think you should be able to make the regular attack work with formations too... Maybe a bit more work.
I mean to look at this after the Unit AI cleanup from the UM rewrite is mostly finished.
I kind of fail to see the point of replacing a constant with a function call. It's more code to read and provides us little.
I don't really like our current capturing, but I don't think we want a "no-capture" game mode in Vanilla 0 A.D., and as @elexis said mods can just remove Capturable.
If capturing is deemed bad enough that people would rather play without it, we should just rework the feature or remove it.
Is the ATTACKGROUND state still necessary?
Weird that it compiled when I tried above. Guessing my compilation setup is a bit too customised for these kind of patches.
Wed, Jun 19
But currently the chech whether a projectile can be launched is performed only once, so we would need a check per projectile I guess?
Yes it's a paradigm change, introduced mostly by D1171 a said above. Launching multiple projectiles is unsupported, but it could/should be added. But I think it'd be better first to make Projectile handle more things.
Forgot about melee attacks... But my above point still stands that we might want attacks that generate more than one projectile, and then these projectiles could have different range behaviour. We might need range at two different levels again.
Tue, Jun 18
Looks good - I'll test when I can and commit it.
In light of D1938, I think instead this should be an attribute to existing damage types.
This indeed looks much better. I kinda wish we could get rid of the Foundation special-ness but it sounds more annoying right now.
Thought about this overnight: this most likely breaks some code think attacking will change the held position kept by other components.
So probably I just need clean things up using orders.
Mon, Jun 17
I was looking into how we could merge this behaviour with the gathering LastPos, and after some thinking my initial diff was pretty poor. UnitMotion waypoints are OK but might not exist, and this is more of a UnitAI behaviour anyways.
Given how Gathering handles this, I should probably just cache a "last known position" variable in unit Motion updated each turn.
This seems to be the cleanest we'll get.
Sun, Jun 16
I think to be quite complete this should make MoveToX set a default long waypoint at the target's position, so that we (almost) always have a position to go to. And perhaps other orders should behave similarly to attack here.
I think it's just a freak test failure since it worked fine on my computer too. Re-uploading to check.
Depending on how much you want to work on this still, I can fix the remaining issues before committing.
Pretty trippy effects. Indeed helpful.
Parsing of "3bogus" is different on libstd and libc, see debates starting in 2013 here: https://bugs.llvm.org/show_bug.cgi?id=17782
Fixes my issue and seems correct to update the locale with regards to blame of this test.
Overall I like this. It cleans up UnitAI nicely too. I've tested it quickly and it works OK in some cases, weirdly in others (check out combat demo).
Also make chasing units not run, since that is no longer needed.
BTW, this could be changed if Move() was split in a pre-move, move, and post-move phase that all units went through sequentially.
It would then look like:
A receives the path to B position's at turn N. It updates its waypoints.
A check if it's in range. [it is currently at B's position at [N-1] — which is now also what B is at, that's the difference with above — so yes]