Page MenuHomeWildfire Games

Unify duplicated texture creation code in the simulation
Changes PlannedPublic

Authored by Sandarac on Jun 23 2017, 8:43 AM.

Details

Reviewers
wraitii
elexis
Summary

The code for initializing textures for SOverlayTexturedLines and SOverlayQuads is duplicated four times in the simulation: in CCmpTerritoryManager, CCmpSelectable (done twice), and CCmpRallyPointRenderer, and this list will grow to include another component with D555 when range-overlay rendering is moved out of CCmpSelectable.

This code involves:

  • Creating a CTextureProperties object for both the mask and base for the texture.
  • Calling SetWrap and SetMaxAnisotropy on the CTextureProperties objects (using the exact same arguments for SetWrap in all mentioned components).
  • Invoking the texture manager (through g_Renderer) twice to create the mask and base.

An option would possibly be to move the code to a file/class in the simulation2/helpers directory, but:

  • The only current "helper" class that is relevant (in Render.cpp/.h) does not access the renderer directly, as it only deals with calculating f.e. line segments.
  • This would still require using the graphics class CTextureProperties directly in the simulation (and adding a bunch of includes from the graphics code).

It is best to try to keep the sim independent of the graphics/rendering code as much as possible (i.e. minimize the need to access the renderer directly in the sim).

All this justifies adding a new method to the graphics code to reduce the duplication, in the part of the code that already deals directly with CTextureProperties and texture creation. These simulation components still need to have access to the renderer in order to check if it is initialized before calculating their lines/quads.

Test Plan

Read the code to verify that no external functionality has changed. Check that all of the duplicated code in the simulation used for creating SOverlayTexturedLines and SOverlayQuads has been moved to the new method. Possibly decide on a better name for the new method.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 2382
Build 3977: Vulcan BuildJenkins
Build 3976: arc lint + arc unit

Event Timeline

Sandarac created this revision.Jun 23 2017, 8:43 AM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jun 23 2017, 8:44 AM
Vulcan added a subscriber: Vulcan.Jun 23 2017, 9:32 AM

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!
Checking XML files...

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

wraitii requested changes to this revision.Jun 24 2017, 10:58 AM
wraitii added a subscriber: wraitii.

I like the idea, but there's really nothing specific here about overlays. I'd refactor this into an overload of CreateTexture (i.e. a no-argument version or something along those lines). You'd have to call it twice, but then it'd be slightly more generic.

If you were to do that, you might check most existing calls to CreateTexture because I'm quite sure that many other cases could be slightly abstracted away.

This revision now requires changes to proceed.Jun 24 2017, 10:58 AM
Sandarac planned changes to this revision.Jun 25 2017, 10:30 AM
In D672#26968, @wraitii wrote:

I like the idea, but there's really nothing specific here about overlays. I'd refactor this into an overload of CreateTexture (i.e. a no-argument version or something along those lines). You'd have to call it twice, but then it'd be slightly more generic.
If you were to do that, you might check most existing calls to CreateTexture because I'm quite sure that many other cases could be slightly abstracted away.

Alright, but an overload of CreateTexture would surely still need arguments for the texture path, mask path and the MaxAnisotropy. Also, most non-overlay textures don't have the call to SetMaxAnisotropy().

elexis resigned from this revision.Dec 12 2017, 8:21 PM

@vladislavbelov you might want to commandeer this. Sandarac is long gone sadly but itd still be good to have.

source/simulation2/components/CCmpTerritoryManager.cpp
576

Can we load a special template instead of Hardcoded this string ?