<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Nov 29, 2012 at 3:19 PM, Neil Roberts <span dir="ltr"><<a href="mailto:neil@linux.intel.com" target="_blank">neil@linux.intel.com</a>></span> wrote:<br> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">Robert Bragg <<a href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> writes:<br> <br> > static void<br> > _cogl_matrix_stack_free (CoglMatrixStack *stack)<br> > {<br> > - _cogl_matrix_entry_unref (stack->last_entry);<br> > + cogl_matrix_entry_unref (stack->last_entry);<br> > + cogl_matrix_entry_ref (&stack->context->identity_entry);<br> <br> </div>Shouldn't that be an unref?<br></blockquote><div><br></div><div>ah, yep right.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"><br> > +CoglMatrixEntry *<br> > +cogl_matrix_stack_get_entry (CoglMatrixStack *stack)<br> > +{<br> > + return cogl_matrix_entry_ref (stack->last_entry);<br> > +}<br> <br> </div>I think it would be better if this didn't take a reference. There is no<br> precedent for a getter that takes a reference, is there? It seems like<br> if it needs to take a ref it should have a different name.<br></blockquote><div><br></div><div>For some reason I'm surprised that's the standard convention but yeah checking lots of examples in Gnome, and also looking at cogl_pipeline_get_layer_texture it looks like this should be changed yeah.</div> <div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <div class="im"><br> > CoglBool<br> > -_cogl_matrix_entry_has_identity_flag (CoglMatrixEntry *entry)<br> > +cogl_matrix_entry_is_identity (CoglMatrixEntry *entry)<br> <br> </div>We've discussed this before, but ‘is_identity’ doesn't seem like a good<br> name because that's not really what it does if it can have false<br> negatives. But well, I guess it's just bike-shedding and I can't think<br> of a better name so that's fine by me.<br></blockquote><div><br></div><div>yeah I recall talking about this before. For some reason the _has_identity flag seemed pretty obscure naming for the public api when there is nothing else to explain that the entry has associated "flags".</div> <div><br></div><div>I don't think the name is to bad along with documentation explaining that it can return false negative results.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"> <br> I also have some minor corrections to the docs which I'll just attach as<br> a diff:<br> <br> diff --git a/cogl/cogl-matrix-stack.h b/cogl/cogl-matrix-stack.h<br> index 5d1848a..0923b81 100644<br> --- a/cogl/cogl-matrix-stack.h<br> +++ b/cogl/cogl-matrix-stack.h<br> @@ -358,7 +358,7 @@ cogl_matrix_stack_frustum (CoglMatrixStack *stack,<br> <div class="im"> * Replaces the current matrix with a perspective matrix based on the<br> </div> * provided values.<br> <div class="im"> *<br> - * <note>You should be careful not to have to great a @z_far / @z_near<br> </div>+ * <note>You should be careful not to have too great a @z_far / @z_near<br> <div class="im"> * ratio since that will reduce the effectiveness of depth testing<br> </div><div class="im"> * since there wont be enough precision to identify the depth of<br> </div><div class="im"> * objects near to each other.</note><br> </div>@@ -528,7 +528,7 @@ cogl_is_matrix_stack (void *object);<br> <div class="im"> * @z: (out): The destination for the z-component of the translation<br> </div> *<br> <div class="im"> * Determines if the only difference between two transforms is a<br> - * translation and if so returns what the @x, @y, and z@ components of<br> </div>+ * translation and if so returns what the @x, @y, and @z components of<br> * the translation are.<br> *<br> <div class="im"> * If the difference between the two translations involves anything<br> </div>@@ -571,7 +571,7 @@ cogl_matrix_entry_is_identity (CoglMatrixEntry *entry);<br> <div class="im"> * returning %TRUE if they are equal or %FALSE otherwise.<br> </div> *<br> <div class="im"> * <note>In many cases it is unnecessary to use this api and instead<br> - * direct pointer comparisons of entries is good enough and much<br> </div>+ * direct pointer comparisons of entries are good enough and much<br> * cheaper too.</note><br> *<br> <div class="im"> * Return value: %TRUE if @entry0 represents the same transform as<br></div></blockquote><div><br></div><div>thanks for this doc fixes, I'll fold those back into my patch before landing.</div><div><br> </div><div>thanks,</div><div>- Robert</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"> <br> </div>Regards,<br> - Neil<br> ---------------------------------------------------------------------<br> Intel Corporation (UK) Limited<br> Registered No. 1134945 (England)<br> Registered Office: Pipers Way, Swindon SN3 1RJ<br> VAT No: 860 2173 47<br> <br> This e-mail and any attachments may contain confidential material for<br> the sole use of the intended recipient(s). Any review or distribution<br> by others is strictly prohibited. If you are not the intended<br> recipient, please contact the sender and delete all copies.<br> _______________________________________________<br> Cogl mailing list<br> <a href="mailto:Cogl@lists.freedesktop.org">Cogl@lists.freedesktop.org</a><br> <a href="http://lists.freedesktop.org/mailman/listinfo/cogl" target="_blank">http://lists.freedesktop.org/mailman/listinfo/cogl</a><br> </blockquote></div><br></div>