Added multiples UVs to animated model
ClosedPublic
Actions

Authored by vladislavbelov on May 7 2017, 2:25 PM.

Details

Reviewers

wraitii
Stan

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rP21291: Added multiples UVs to animated model

Trac Tickets

#2680

Summary

Replaces m_UV by m_UVs to support multiple UV per model, it could be used for AO of not really dynamic objects and etc.

I've not lost performance, but we should test it on different hardware to be sure of making this change.

Test Plan

Run game
Turn on profiler
Run patched game
Turn on profiler
Compare results

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

vladislavbelov created this revision.May 7 2017, 2:25 PM

Thanks for working on this.

source/renderer/HWLightingModelRenderer.cpp
58 ↗	(On Diff #1717)	i++ → ++i
68 ↗	(On Diff #1717)	i++ → ++i
71 ↗	(On Diff #1717)	Maybe could be one line dunno.
267 ↗	(On Diff #1717)	Wrong identation I think
270 ↗	(On Diff #1717)	Wrong identation I think

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/1057/ for more details.

Harbormaster completed remote builds in B1517: Diff 1717.May 7 2017, 9:19 PM

vladislavbelov updated the Trac tickets for this revision.May 8 2017, 12:11 PM

wowgetoffyourcellphone added a subscriber: wowgetoffyourcellphone.May 30 2017, 9:23 AM

Looks good to me. @wraitii or @Itms should check for c++ correctness but I'm okay with it.

This revision is now accepted and ready to land.Aug 7 2017, 1:39 PM

In D448#30300, @Stan wrote:

I'm okay with it.

Reviewers should accept if the patch was reviewed (which includes understanding the code and testing for issues in the design / approach) and tested.
The patch description could be a bit more elaborate so that even readers who aren't familiar with the affected code can become convinced that the patch is as good as intended.

Looks good to me too, consider comments before committing.