Page MenuHomeWildfire Games

Removes CPU lighting after no FFP
ClosedPublic

Authored by vladislavbelov on Wed, Jan 13, 12:30 AM.

Details

Reviewers
None
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP24599: Removes CPU lighting after no FFP
Summary

FFP removed, so we don't need to spend CPU time on that.

Test Plan
  1. Apply the patch and compile the game
  2. Make sure that everything works as before
  3. Check that CPU lighting doesn't still present somewhere in changed code

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

Build is green

builderr-debug-macos.txt
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgraphics_dbg.a(precompiled.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: ../../../binaries/system/libgui_dbg.a(precompiled.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreAudio.framework/CoreAudio.tbd and library file /System/Library/Frameworks//CoreAudio.framework/CoreAudio are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox.tbd and library file /System/Library/Frameworks//AudioToolbox.framework/AudioToolbox are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//ForceFeedback.framework/ForceFeedback.tbd and library file /System/Library/Frameworks//ForceFeedba

See https://jenkins.wildfiregames.com/job/macos-differential/2816/display/redirect for more details.

vladislavbelov requested review of this revision.Wed, Jan 13, 1:07 AM
Stan added a subscriber: Stan.Wed, Jan 13, 1:12 AM
Stan added inline comments.
source/renderer/HWLightingModelRenderer.cpp
101 ↗(On Diff #15219)

Maybe?

source/renderer/Renderer.cpp
172 ↗(On Diff #15219)

False positives ?

553 ↗(On Diff #15219)

Maybe this could be way higher in the code, so we don't waste time checking everywhere

  1. Succeeded.
  2. No oddities found.
  3. Grepping for cpuLighting didn't find any occurences ;)
vladislavbelov added inline comments.Wed, Jan 13, 8:25 AM
source/renderer/HWLightingModelRenderer.cpp
101 ↗(On Diff #15219)

struct, mostly as POD.

source/renderer/Renderer.cpp
172 ↗(On Diff #15219)

It'd be right if it'd return const char*, but it returns CStr. So the return value will be constructed before the buf will be destroyed.

553 ↗(On Diff #15219)

We're already checking. It's not a regression. Just a notice to validate the place after removing RenderPath::FIXED from real code.

Stan added inline comments.Wed, Jan 13, 8:33 AM
source/renderer/HWLightingModelRenderer.cpp
101 ↗(On Diff #15219)

What's the advantage of the struct here since there will be only one field? Maybe we should use just the raw pointer everywhere?

source/renderer/Renderer.cpp
172 ↗(On Diff #15219)

Ah makes sense

553 ↗(On Diff #15219)

Makes sense.

vladislavbelov added inline comments.Wed, Jan 13, 8:47 AM
source/renderer/HWLightingModelRenderer.cpp
101 ↗(On Diff #15219)

Possible future extension.

Stan added a comment.Wed, Jan 13, 2:15 PM

Will it make a profileable difference?

In D3346#148800, @Stan wrote:

Will it make a profileable difference?

No visible differences.

Stan added a comment.Wed, Jan 13, 4:32 PM

We're in FF btw

In D3346#148906, @Stan wrote:

We're in FF btw

I've seen, but that patch was already planned, so no problem.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Jan 13, 10:04 PM
This revision was automatically updated to reflect the committed changes.