#1865 closed defect (fixed)
[PATCH] Rally point marker detached from rally point line
| Reported by: | zoot | Owned by: | ben |
|---|---|---|---|
| Priority: | Release Blocker | Milestone: | Alpha 13 |
| Component: | UI & Simulation | Keywords: | patch |
| Cc: | sbte | Patch: |
Description (last modified by )
Steps to reproduce:
- Select a civic center.
- Repeatedly place a new rally point.
What happens: Occasionally (every 10 times or so), the rally point line will update, but the rally point flag will not.
What ought to happen: Both the line and the flag should update.
Attachments (5)
Change History (27)
by , 11 years ago
| Attachment: | screenshot0260.jpeg added |
|---|
comment:1 by , 11 years ago
| Description: | modified (diff) |
|---|
comment:2 by , 11 years ago
| Cc: | added |
|---|
by , 11 years ago
| Attachment: | 0004-Always-wait-for-at-least-one-interpolation-before-sk.patch added |
|---|
comment:3 by , 11 years ago
Sorry for the amount of bugs my patch has caused. I hope the attached patch fixes it for you.
comment:4 by , 11 years ago
| Component: | Core engine → UI & Simulation |
|---|---|
| Keywords: | patch review added |
| Milestone: | Backlog → Alpha 13 |
| Summary: | Rally point marker detached from rally point line → [PATCH] Rally point marker detached from rally point line |
comment:5 by , 11 years ago
Sbte: gave it a shot. It seems to happen less often but still happens sometimes. Note quite sure what you're trying to do with that patch, it seems to me like this would cancel some of the optimization?
comment:6 by , 11 years ago
I don't have time to look into this any further right now, but I can explain what the patch does.
The first line makes sure that something is actually not rendered again when a position did not change. This is actually a small optimization wrt the previous approach.
The second line makes sure that, when something is moved using a Jump (like setting a rally point), the interpolation will happen again at least once.
I'm not sure if this is possible, but a race condition could be that both the interpolate case and the new turn case in Position are called before the interpolate in VisualActor, and hence, it is still not rendered again. Maybe this is what you observe.
comment:7 by , 11 years ago
wraitii, I tried setting the rally point probably a 1000 times on different maps, and nothing went wrong for me with my patch applied. Really weird.
comment:9 by , 11 years ago
Ok, now I'm a bit more into the code, I came up with a totally different approach that seems to speed things up even more, and probably also fixes this bug. I didn't test the patch a lot, but I'm heading to bed now, and I'm just posting the patch here so other people can have a look at it. I did test, however, for all bugs that were encountered with my previous patches, and I didn't see any of those.
by , 11 years ago
| Attachment: | 0002-Move-checking-for-reinterpolation-after-a-position-h.patch added |
|---|
comment:10 by , 11 years ago
I tested both patches and they both seem to fix the problem on my system.
comment:11 by , 11 years ago
About the second patch, could you use the MT_TurnStart message instead of using the global turn manager? Seems like that would be cleaner.
comment:12 by , 11 years ago
| Priority: | Should Have → Release Blocker |
|---|
comment:13 by , 11 years ago
historic_bruno: I tried the approach using MT_TurnStart before I used this approach, but MT_TurnStart seemed to be a lot slower. I thought this was because of the need to iterate over more elements in the ComponentManager. But I can investigate this some more.
by , 11 years ago
| Attachment: | 0002-Move-checking-for-reinterpolation-after-a-position-h.2.patch added |
|---|
Version 2 of the patch using MT_TurnStart
comment:14 by , 11 years ago
Added a new version that uses MT_TurnStart. With high FPS the change is not really noticeable, but I imagine that with low FPS, it might be worse.
The slowdown I originally saw is because I connect to MT_PositionChanged, which makes PostMessage slower. This is however solved by moving the check in VisualActor forward, because the speedup caused by not creating the cmpPosition for static objects is bigger.
comment:15 by , 11 years ago
Replying to wraitii:
I'll give it another try tomorrow.
Can you still reproduce this with the last patch? I don't seem able to reproduce it with the latest patch anymore.
comment:16 by , 11 years ago
Thanks, the patch looks better. I think the special cases can be eliminated though. One thing is to use MT_TerrainChanged (which is already handled by CmpVisualActor) to detect terrain changes in Atlas and assume the position changed, instead of using the gameloop hack. That should also handle future cases of the terrain changing in-game (could be improved by calculating if the terrain change actually affected this entity). The other is to only require interpolation when the construction progress is changed, instead of needlessly interpolating even when it hasn't changed.
I'm attaching a patch with these suggestions. If there are no objections, it can be committed.
comment:17 by , 11 years ago
I also changed m_Interpolated to m_NeedsInterpolation (and inverted the logic), as I think that's clearer.
by , 11 years ago
| Attachment: | 0003-Move-checking-for-reinterpolation-after-a-position-h.2.patch added |
|---|
Removed checks for special cases, update them appropriately
comment:18 by , 11 years ago
Patch failed to apply properly but that was easily fixed, might come from some changes of mine.
Historic_bruno's patch seems to work cleanly. haven't been able to get the error in the ticket, nor any other one. I agree with him that doing this in the visual actor is better. Atlas seems to work too.
comment:21 by , 11 years ago
| Keywords: | review removed |
|---|
I suspect this is caused by setting the rally point just before a new simulation turn. That way m_PositionChanged in CCmpPosition is reset before Interpolate in CCmpVisualActor is called, which would explain why this just happens sometimes (and only after r13223).
I'm not entirely sure how to solve this nicely though.