Issue 6423066: Add GL Uniform Manager

Issue 6423066: Add GL Uniform Manager (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 4 months ago by bsalomon

Modified:
12 years, 4 months ago

Reviewers:
TomH

CC:
skia-review_googlegroups.com, Stephen White

Base URL:
http://skia.googlecode.com/svn/trunk/

Visibility:
Public.

Patch Set 1 #

Patch Set 2 : update #

Total comments: 9

Created: 12 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+577 lines, -513 lines)			Patch
M	gyp/gpu.gyp	View	1	1 chunk	+3 lines, -0 lines	0 comments	Download
M	src/effects/SkLightingImageFilter.cpp	View	1	27 chunks	+64 lines, -163 lines	0 comments	Download
M	src/gpu/effects/GrColorTableEffect.cpp	View	1	1 chunk	+1 line, -5 lines	0 comments	Download
M	src/gpu/effects/GrConvolutionEffect.cpp	View	1	7 chunks	+8 lines, -29 lines	0 comments	Download
M	src/gpu/effects/GrGradientEffects.cpp	View	1	9 chunks	+11 lines, -40 lines	0 comments	Download
M	src/gpu/effects/GrMorphologyEffect.cpp	View	1	6 chunks	+7 lines, -19 lines	0 comments	Download
M	src/gpu/effects/GrSingleTextureEffect.cpp	View	1	1 chunk	+0 lines, -4 lines	0 comments	Download
M	src/gpu/effects/GrTextureDomainEffect.cpp	View	1	7 chunks	+8 lines, -28 lines	0 comments	Download
M	src/gpu/gl/GrGLProgram.h	View	1	6 chunks	+29 lines, -35 lines	0 comments	Download
M	src/gpu/gl/GrGLProgram.cpp	View	1	13 chunks	+37 lines, -110 lines	0 comments	Download
M	src/gpu/gl/GrGLProgramStage.h	View	1	1 chunk	+1 line, -7 lines	0 comments	Download
M	src/gpu/gl/GrGLProgramStage.cpp	View	1	1 chunk	+1 line, -7 lines	0 comments	Download
M	src/gpu/gl/GrGLShaderBuilder.h	View	1	4 chunks	+20 lines, -21 lines	0 comments	Download
M	src/gpu/gl/GrGLShaderBuilder.cpp	View	1	5 chunks	+21 lines, -14 lines	0 comments	Download
A	src/gpu/gl/GrGLUniformHandle.h	View		1 chunk	+16 lines, -0 lines	0 comments	Download
A	src/gpu/gl/GrGLUniformManager.h	View	1	1 chunk	+76 lines, -0 lines	4 comments	Download
A	src/gpu/gl/GrGLUniformManager.cpp	View		1 chunk	+247 lines, -0 lines	4 comments	Download
M	src/gpu/gl/GrGpuGL.h	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
M	src/gpu/gl/GrGpuGL_program.cpp	View	1	11 chunks	+25 lines, -29 lines	1 comment	Download

Messages

Total messages: 4

Expand All Messages | Collapse All Messages

TomH

LGTM modulo nits. Sorry about the slow turnaround on your GLUM review. http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager.cpp File src/gpu/gl/GrGLUniformManager.cpp ...

12 years, 4 months ago (2012-07-25 17:26:18 UTC) #3

bsalomon

12 years, 4 months ago (2012-07-25 19:54:55 UTC) #4

Closed with r4758. Assertion issues fixed in r4759 and r4761.

http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager...
File src/gpu/gl/GrGLUniformManager.cpp (right):

http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager...
src/gpu/gl/GrGLUniformManager.cpp:12: #define ASSERT_ARRAY(UNI, OFFSET, COUNT) \
On 2012/07/25 17:26:18, TomH wrote:
> Nit: useful assert; can we come up with a more descriptive name?

Done.

http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager...
src/gpu/gl/GrGLUniformManager.cpp:32: if (kUnusedUniform != uni.fFSLocation) {
On 2012/07/25 17:26:18, TomH wrote:
> These two if() statements appear over and over - is it worth creating a couple
> of helper functions, or even macros?

I think to make it worthwhile we'd need variadic macros.

http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager.h
File src/gpu/gl/GrGLUniformManager.h (right):

http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager...
src/gpu/gl/GrGLUniformManager.h:30: /** Functions for uploading uniform values.
The varities ending in v can be used to upload to an
On 2012/07/25 17:26:18, TomH wrote:
> typo: extra "to an"

Done.

http://codereview.appspot.com/6423066/diff/3001/src/gpu/gl/GrGLUniformManager...
src/gpu/gl/GrGLUniformManager.h:42: // matrices are column-major, the first
three upload a single matrix, the latter three upload
On 2012/07/25 17:26:18, TomH wrote:
> typo: extra "upload"

Done.

Expand All Messages | Collapse All Messages