Differential D1158
Fix selection rings while garrisoning Authored by temple on Dec 18 2017, 6:52 PM.
Details
Selection rings stay active for a fraction of a second after units garrison. It's a bit distracting once you're aware of it. Instead of continuing early if the unit's not in the world, we instead first do UpdateVisibility which sets the visibility in Selectable and then continues (since visibility is hidden). The position changed message in Selectable isn't needed anymore (history at #2627). Fixes the persistent hero ring described here: https://code.wildfiregames.com/rP20622#26087 Videos of the difference:
Diff Detail
Event TimelineComment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Comment Actions Thank you very much for taking a look and providing a fix! It works for me. I have a question, now the heroes are consistent with the rest of the units, in that the ring stays for a turn before disappearing, whereas before, the hacky code made the star disappear immediately for heroes and a turn too late for units. Would it be possible to have the immediate disappearance of the star for all units or would that require tedious work? It's just a graphical nitpick, I noticed that because I was specifically looking at the selection rings. Regarding the code, the a = b = c was a bit borderline, but passing the side effect of a copy to the function is really not something we should do, so I'm afraid we need to add a new line and braces. Finally do you think you could add some tests to that component while you're at it? Obviously they wouldn't test the graphical stuff but testing thoroughly the internal state of the component would help a lot I believe. Comment Actions I noticed that too when doing diplomacy colors. (The rings stay for CCmpSelectable::FADE_DURATION = 0.3 seconds, not just a turn.) Think I found the problem: there's an early continue in UnitRenderer when units aren't in the world, so it never does UpdateVisibility which would call the SetVisibility function of Selectable which would hide the selection rings. Comment Actions Successful build - Chance fights ever on the side of the prudent. Updating workspaces... Build (release)... Build (debug)... Running release tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK! Checking XML files... Comment Actions
Sounds reasonable. Can we deduce a recipe to reproduce that ingame? Comment Actions If you mean reproducing the bug, you just need to garrison units that are selected. When the units disappear the selection rings stay on the ground for an extra 0.3 seconds. (It doesn't depend on the game speed, so you have to change the constant if you want it longer and thus more noticeable. But 0.3s is noticeable if you're looking for it.) Comment Actions When we garrison it does fade, however we quit early in UpdateDynamicOverlay in Selection because we don't have a position anymore, so we don't update the alpha value of the overlay. At the same time we still do RenderSubmit, where we then use the m_UnitOverlay that has the old m_Color values. That's why the visible selection ring alpha stays the same rather than fading. It disappears when the fading finishes and we stop doing RenderSubmit. In the patch we set m_Visible = false, so when we do RenderSubmit now it doesn't show the overlay, even though m_Color.a > 0 while it fades. Comment Actions Good, already wanted to ask if it wouldn't be cleaner to set some visbility to false rather than setting the transparency to 0. Comment Actions There's the hero bug where the selection ring stays forever (until you ungarrison), and then there's the unit bug where the selection ring stay for 0.3 seconds. I'm taking it that you're talking about the first bug here. I don't have a21 and earlier, so I can't verify that steps 1-3 fail to reproduce it (step 3 should be deselect the hero). (There was rP19732 and rP19777 but I don't think they'd affect this.) (Let me download a21 and test it.) Comment Actions I could just reproduce it in alpha 21:
The last step isn't needed in >= a22. Comment Actions Okay, I grabbed a21. I don't know what changed, I guess it could be the commits I mentioned, but I don't really want to spend any time investigating. Comment Actions Successful build - Chance fights ever on the side of the prudent. Link to build: https://jenkins.wildfiregames.com/job/differential/29/display/redirect Comment Actions
Then why is it still present in the code and code executed if it arrived? Tested and fixes the bug. Thanks temple for having fixed this bug after so much back and forth in the wrong component!
| ||||||||||||||