Closed
Bug 975772
Opened 11 years ago
Closed 11 years ago
cleanup RectTriangles and allow DrawQuads to take triangles or triangle strips
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: gal, Assigned: gal)
References
Details
Attachments
(1 file, 5 obsolete files)
13.21 KB,
patch
|
bjacob
:
review+
|
Details | Diff | Splinter Review |
I am about to use large meshes via the same path we pipe RectTriangles through and we should really be using triangle strips here. Degenerate triangles are used to emit multiple quads as a single triangle strip.
Assignee | ||
Comment 1•11 years ago
|
||
This implements the above and also optimizes the way we allocate vertices and generally removes a bunch of redundant and repetitive code.
Assignee: nobody → gal
Assignee | ||
Comment 2•11 years ago
|
||
Actually never mind. GL_TRIANGLES is just as compact for this specific use case. I will instead generalize the DrawQuads path so it can do triangles and triangle strips and merely clean up DecomposeIntoNoRepeatTriangles and land that and that unblocks me to use strips where I want to.
Assignee | ||
Updated•11 years ago
|
Summary: optimize RectTriangles to use triangle strips → cleanup RectTriangles and allow DrawQuads to take triangles or triangle strips
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #8380252 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8380331 -
Flags: review?(vladimir)
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #8380331 -
Attachment is obsolete: true
Attachment #8380331 -
Flags: review?(vladimir)
Assignee | ||
Updated•11 years ago
|
Attachment #8384152 -
Flags: review?(vladimir)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #8384152 -
Attachment is obsolete: true
Attachment #8384152 -
Flags: review?(vladimir)
Assignee | ||
Updated•11 years ago
|
Attachment #8384157 -
Flags: review?(bjacob)
Comment 6•11 years ago
|
||
Comment on attachment 8384157 [details] [diff] [review]
patch
Review of attachment 8384157 [details] [diff] [review]:
-----------------------------------------------------------------
Lots of great code simplification here, just one blocking nit about the new void* pointers which look like they could be replaced by something more typed, but tell me if I'm missing something.
::: gfx/gl/DecomposeIntoNoRepeatTriangles.cpp
@@ +11,5 @@
> namespace gl {
>
> +template <typename T>
> +static void
> +AddTriangle(InfallibleTArray<T>& array, GLfloat x0, GLfloat y0, GLfloat x1, GLfloat y1)
See comment in DecomposeIntoNoRepeatTriangles.h: at this point, we're better off unifying the two 'coord' structs and de-templatifying this function.
Also, the name of this function can't be quite right: this function adds 6 vertices, visibly representing 1 quad subdivided into 2 triangles, so it shouldn't be named "AddTriangle" singular. "AppendRectToCoordArray" would be more to the point?
@@ +42,5 @@
> + if (mVertexCoords.Length() == 6 &&
> + mVertexCoords[0].x == 0.0f &&
> + mVertexCoords[0].y == 0.0f &&
> + mVertexCoords[5].x == 1.0f &&
> + mVertexCoords[5].y == 1.0f) {
Brace { on new line after multi-line if(...) condition.
I do like having this computed rather than returning additional state, mIsSimpleQuad and mTextureTransform, that was redundant. Redundant state is evil.
::: gfx/gl/DecomposeIntoNoRepeatTriangles.h
@@ +51,4 @@
> }
>
> typedef struct { GLfloat x,y; } vert_coord;
> + typedef struct { GLfloat x,y; } tex_coord;
I think at this point it makes more sense to just unify these two structs. I was all for strong typing, but at this point, we're making them usable interchangeably by making AddTriangles templated in the coord type, which negates that.... so better have only one type, and de-templatify AddTriangles in DecomposeIntoNoRepeatTriangles.cpp.
::: gfx/gl/GLBlitTextureImageHelper.cpp
@@ +135,5 @@
> DecomposeIntoNoRepeatTriangles(srcSubRect, realTexSize, rects);
>
> // now put the coords into the d[xy]0 .. d[xy]1 coordinate space
> // from the 0..1 that it comes out of decompose
> + RectTriangles::vert_coord* v = (RectTriangles::vert_coord*)rects.vertCoords();
While you're here please remove this C-style cast here.
Since we do have a use for that vertCoords() method returning float* below, we might want to have another, differently named, getter for getting at the array of coord structs; and it would be a lot safer to return a Array& reference than a plain coord* pointer.
::: gfx/layers/opengl/CompositorOGL.cpp
@@ +81,5 @@
> ShaderProgramOGL *aProg,
> + GLenum aMode,
> + GLsizei aElements,
> + void *aVertCoords,
> + void *aTexCoords)
Oh no, please no new void* pointers here!
Attachment #8384157 -
Flags: review?(bjacob) → review-
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8384157 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8385755 -
Flags: review?(bjacob)
Comment 8•11 years ago
|
||
The void* pointers seem to still be there! If there is a strong reason for them, please add a comment.
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8385755 -
Attachment is obsolete: true
Attachment #8385755 -
Flags: review?(bjacob)
Assignee | ||
Comment 10•11 years ago
|
||
Comment on attachment 8385770 [details] [diff] [review]
patch
Will be more apparent with the next patch. I will just fold it in later.
Attachment #8385770 -
Flags: review?(bjacob)
Comment 11•11 years ago
|
||
Comment on attachment 8385770 [details] [diff] [review]
patch
Review of attachment 8385770 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/gl/DecomposeIntoNoRepeatTriangles.cpp
@@ +14,5 @@
> +RectTriangles::AppendRectToCoordArray(InfallibleTArray<coord>& array,
> + GLfloat x0, GLfloat y0,
> + GLfloat x1, GLfloat y1)
> +{
> + coord* v = array.AppendElements(6);
Rather than operating on a raw pointer like this, it would be safer to use operator[] on 'array' directly, so as to be covered by assertions, but I don't mind strongly.
Attachment #8385770 -
Flags: review?(bjacob) → review+
Assignee | ||
Comment 12•11 years ago
|
||
(In reply to Benoit Jacob [:bjacob] from comment #11)
> Comment on attachment 8385770 [details] [diff] [review]
> patch
>
> Review of attachment 8385770 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: gfx/gl/DecomposeIntoNoRepeatTriangles.cpp
> @@ +14,5 @@
> > +RectTriangles::AppendRectToCoordArray(InfallibleTArray<coord>& array,
> > + GLfloat x0, GLfloat y0,
> > + GLfloat x1, GLfloat y1)
> > +{
> > + coord* v = array.AppendElements(6);
>
> Rather than operating on a raw pointer like this, it would be safer to use
> operator[] on 'array' directly, so as to be covered by assertions, but I
> don't mind strongly.
In this particular case its possible to visually confirm correctness. Calling repeatedly overloaded operators that are defined elsewhere and might independently change seems more risky (from a performance perspective in particular).
Assignee | ||
Comment 13•11 years ago
|
||
This doesn't have the right checkin comment set. I will try to get to that later today but if someone beats me to it and lands it, I owe someone a drink :)
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac4f9ca4fb27
Andreas, can you please make sure you have hg configured to generate patches with commit information? Makes life much easier with the checkin-neededs :)
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•