Refactor range overlay renderering to allow showing ranges in building previews
ClosedPublic
Actions

Authored by Sandarac on May 26 2017, 4:50 AM.

Details

Reviewers

elexis
vladislavbelov

Commits

rP20622: Render the range visualization of auras, heal and attack component in a…

Summary

Moves the logic to CmpRangeOverlayRenderer. This will allow showing the aura and attack ranges in building previews (it was wanted in particular for attack ranges, and adding attack range visualization will be trivial after this).

Also, some structs have been moved out of Cmpselectable to Overlay.h, where they fit better and can be used by both classes.

Test Plan

Try placing the Pillar of Ashoka in game to see the aura, notice the code doesn't change any other functionality.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Sandarac created this revision.May 26 2017, 4:50 AM

Build has FAILED

Updating workspaces.
Build (release)...
In file included from ../../../source/simulation2/components/ICmpRangeOverlayRenderer.h:22:0,
                 from ../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp:20:
../../../source/simulation2/components/ICmpRangeOverlayRenderer.h: In static member function ‘static int ICmpRangeOverlayRenderer::GetInterfaceId()’:
../../../source/simulation2/system/Interface.h:26:39: error: ‘IID_RangeOverlayRenderer’ was not declared in this scope
  static int GetInterfaceId() { return IID_##iname; }
                                       ^
../../../source/simulation2/components/ICmpRangeOverlayRenderer.h:37:2: note: in expansion of macro ‘DECLARE_INTERFACE_TYPE’
  DECLARE_INTERFACE_TYPE(RangeOverlayRenderer)
  ^
In file included from ../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp:30:0:
../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp: In member function ‘virtual int CCmpRangeOverlayRenderer::GetComponentTypeId() const’:
../../../source/simulation2/system/Component.h:49:10: error: ‘CID_RangeOverlayRenderer’ was not declared in this scope
   return CID_##cname; \
          ^
../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp:40:2: note: in expansion of macro ‘DEFAULT_COMPONENT_ALLOCATOR’
  DEFAULT_COMPONENT_ALLOCATOR(RangeOverlayRenderer)
  ^
In file included from ../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp:30:0:
../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp: In function ‘void RegisterComponentType_RangeOverlayRenderer(CComponentManager&)’:
../../../source/simulation2/system/Component.h:33:60: error: ‘CID_RangeOverlayRenderer’ was not declared in this scope
   mgr.RegisterComponentType(CCmp##cname::GetInterfaceId(), CID_##cname, CCmp##cname::Allocate, CCmp##cname::Deallocate, #cname, CCmp##cname::GetSchema()); \
                                                            ^
../../../source/simulation2/components/CCmpRangeOverlayRenderer.cpp:244:1: note: in expansion of macro ‘REGISTER_COMPONENT_TYPE’
 REGISTER_COMPONENT_TYPE(RangeOverlayRenderer)
 ^
make[1]: *** [obj/simulation2_Release/CCmpRangeOverlayRenderer.o] Error 1
make: *** [simulation2] Error 2
make: *** Waiting for unfinished jobs....

Link to build: http://jw:8080/job/phabricator/1364/
See console output for more information: http://jw:8080/job/phabricator/1364/console

Harbormaster failed remote builds in B1972: Diff 2209!May 26 2017, 5:49 AM

Forgot TypeList.h.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1370/ for more details.

Harbormaster completed remote builds in B1980: Diff 2219.May 26 2017, 4:46 PM

Sandarac mentioned this in D568: Visual Attack Range.May 28 2017, 8:38 PM

Sandarac mentioned this in D672: Unify duplicated texture creation code in the simulation.Jun 23 2017, 8:43 AM

vladislavbelov added a reviewer: vladislavbelov.Dec 9 2017, 8:13 PM

No merge conflicts with D672.

binaries/data/mods/public/simulation/templates/template_unit.xml
70 ↗	(On Diff #2219)	A rename from `RangeVisualization` to `RangeOverlayManager` would fit to `RangeOverlayRenderer`.
source/simulation2/components/CCmpRangeOverlayRenderer.cpp
187 ↗	(On Diff #2219)	Above copy&pasta seems acceptable, but we probably still want to move things from Selectable here. (Being able to render a round overlay has nothing to do with being able to select the entity as is evident in the use case implemented here.)
233 ↗	(On Diff #2219)	Bad news is that everything is copied, it should call common code. Good news being that `graphics/Overlays.cpp` is the perfect place for the non-simulation part, because you have moved the one missing data structure to `Overlay.h`. This also helps with splitting simulation and renderer tasks.
source/simulation2/components/CCmpSelectable.cpp
471 ↗	(On Diff #2219)	refs rP19959

building_preview_rebased.patch36 KBDownload

rebased, duplication merged

elexis mentioned this in rP20618: Move EOverlayType and SOverlayDescriptor from the Selectable component to….Dec 9 2017, 8:50 PM

building_preview_rebased.patch33 KBDownload

rebased

It looks like something is missing here, I don't see AngularStepFromChordLen.

source/simulation2/components/CCmpRangeOverlayRenderer.cpp
220 ↗	(On Diff #2219)	`size_t`.
223 ↗	(On Diff #2219)	`site_t`.

elexis mentioned this in D1139: Move from CCmpSelectable::UpdateTexturedLineOverlay to simulation2/helpers/Render.cpp and graphics/Overlay.cpp.Dec 10 2017, 12:17 AM

elexis mentioned this in rP20621: Move selection ring OverlayTexture code from CCmpSelectable….Dec 10 2017, 1:21 AM

rP20618 moved the struct and enum from CCmpSelectable to Overlay.
The copypasta was addressed rP20621.

Thanks a lot for this patch!
It is much cleaner having the rendering of range overlays being done by a range overlay renderer instead of the selection component.
So that was the hidden design flaw in D328 and it's a clean revert of that part and adding the same in the new component as it should have been to begin with.
Perhaps we can eventually make that a single C++ component if it will be easy enough to extend there.

I have done some profiling and could not find a performance malus that would block this important cleanup and feature addition:
The replay is for 20620.

profiling.7z3 MBDownload