Render the range visualization of auras, heal and attack component in a…
AuditedrP20622
Actions

Description

Render the range visualization of auras, heal and attack component in a separate RangeOverlayRenderer component instead of abusing Selectable for that.
This also allows non-selectable entities like building previews to cast range visualizations.

Patch By: Sandarac
Differential Revision: https://code.wildfiregames.com/D555
Refs #3915, #4349, rP19519 / D238.
Comments By: Vladislav

Details

Auditors

Itms

Committed

elexis

Dec 10 2017, 3:41 AM

Differential Revision

D555: Refactor range overlay renderering to allow showing ranges in building previews

Parents

rP20621: Move selection ring OverlayTexture code from CCmpSelectable…

Branches

Unknown

Tags

Unknown

Build Status

Buildable 4053
Build 7117: Trigger Windows Autobuild
Build 7116: Post-Commit Build	Jenkins

Event Timeline

elexis committed rP20622: Render the range visualization of auras, heal and attack component in a….Dec 10 2017, 3:41 AM

elexis mentioned this in rP20627: Rename RangeVisualization component from rP19519 / D238 to RangeOverlayManager….Dec 10 2017, 4:29 AM

Harbormaster completed building B4053: rP20622: Render the range visualization of auras, heal and attack component in a….Dec 10 2017, 4:40 AM

I didn't look at the code closely so I hope I am raising a concern on the correct commit: we have a regression as I experience #2627 again. This commit is the most likely to have reintroduced that (but that refactoring is likely to prevent future appearances of the bug, no pain no gain). Maybe we should write unit tests for RangeOverlayRenderer now that it's a standalone component?

This commit now has outstanding concerns.Dec 17 2017, 10:20 PM

I didn't look at the code closely so I hope I am raising a concern on the correct commit

This is not how it works.
Post how to reproduce a bug and then test which commit introduced it, both are missing here.
I can't reproduce it.

we have a regression as I experience #2627 again.

I did experience #2627 in some matches in a22 and prior releases too but it was never reproducible with visual replays. I still have it in some screenshots.

I don't think this commit can introduce a selection ring bug since it only removed the range overlays from selectable, the selection ring isn't a range overlay and the selectable code wasn't changed.

Here an a22 replay where I got the bug. And I also got this bug many months before aura range visualization, so it's not that either. It has never been fixed correctly / for all situtations.

commands.txt7 KBDownload

(not reproducible in the visual replay (unless it requires some specific GUI selection))

(In advance: There are two ways to remove a concern from a commit, one of them implies reading and understanding it)

In a22 I can almost reliably reproduce the bug by selecting a hero, ordering to garrison an entity, then selecting the garrisoned entity just before the garrisoning occurs.
In a19-22 I can't reproduce it that way, but I know that I have analyzed that issue long before a22 multiple times without reproduce success (as in screenshot and asking others in the chat if they see it too without having it seen in the visual replay).
Someone interested in the bug could be able to determine ways to reproduce the bug by reading the Selectable component and how it's used by the renderer.

elexis requested verification of this commit.Dec 18 2017, 4:25 PM

This commit now requires verification by auditors.Dec 18 2017, 4:25 PM

My bad, sorry for performing the wrong action. I hesitated between this and reopening the ticket, but the recent commit put me on the wrong track, I should have done the latter.

All concerns with this commit have now been addressed.Dec 18 2017, 9:12 PM

Thanks for the audit regardless ;-)

(in case it was one)

elexis mentioned this in rP20961: Revert unintentional function rename in rP20627..Jan 22 2018, 7:03 AM

elexis mentioned this in rP20963: Fix range overlay color after deserialization following rP20622 / D555, refs….Jan 22 2018, 7:50 AM

vladislavbelov added a subscriber: vladislavbelov.Dec 11 2019, 12:41 AM

vladislavbelov added inline comments.

/ps/trunk/source/simulation2/components/CCmpRangeOverlayRenderer.cpp
114	It doesn't seem necessary for static units/buildings. It might make sense to update/recreate overlays only on position/radius/etc change.

vladislavbelov added inline comments.Dec 11 2019, 9:51 AM

/ps/trunk/source/simulation2/components/CCmpRangeOverlayRenderer.cpp
103	This method should be marked as virtual to prevent overloading or missed usage.

I missed in code where are m_EnabledRenderSubmit or m_EnabledInterpolate used?
Looks like they are here just for switch, they can be merged as always or are both enabled or both disabled.
Also that two ifs can be merged.

/ps/trunk/source/simulation2/components/CCmpRangeOverlayRenderer.cpp
158	needInterpolate = m_Enabled needRenderSubmit = m_Enabled needInterpolate = needRenderSubmit
160	if (m_Enabled != m_EnabledInterpolate)
163	m_EnabledInterpolate = m_Enabled
164	already is
167	if (m_Enabled != m_EnabledInterpolate)
170	m_EnabledRenderSubmit = m_Enabled
171	already is