[Cogl] [PATCH] matrix-stack: make CoglMatrixStack public

[Robert Bragg]

On Thu, Nov 29, 2012 at 3:19 PM, Neil Roberts <neil at linux.intel.com> wrote:

> Robert Bragg <robert at sixbynine.org> writes:
>
> >  static void
> >  _cogl_matrix_stack_free (CoglMatrixStack *stack)
> >  {
> > -  _cogl_matrix_entry_unref (stack->last_entry);
> > +  cogl_matrix_entry_unref (stack->last_entry);
> > +  cogl_matrix_entry_ref (&stack->context->identity_entry);
>
> Shouldn't that be an unref?
>

ah, yep right.


>
> > +CoglMatrixEntry *
> > +cogl_matrix_stack_get_entry (CoglMatrixStack *stack)
> > +{
> > +  return cogl_matrix_entry_ref (stack->last_entry);
> > +}
>
> I think it would be better if this didn't take a reference. There is no
> precedent for a getter that takes a reference, is there? It seems like
> if it needs to take a ref it should have a different name.
>

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.


>
> >  CoglBool
> > -_cogl_matrix_entry_has_identity_flag (CoglMatrixEntry *entry)
> > +cogl_matrix_entry_is_identity (CoglMatrixEntry *entry)
>
> We've discussed this before, but ‘is_identity’ doesn't seem like a good
> name because that's not really what it does if it can have false
> negatives. But well, I guess it's just bike-shedding and I can't think
> of a better name so that's fine by me.
>

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".

I don't think the name is to bad along with documentation explaining that
it can return false negative results.


> I also have some minor corrections to the docs which I'll just attach as
> a diff:
>
> diff --git a/cogl/cogl-matrix-stack.h b/cogl/cogl-matrix-stack.h
> index 5d1848a..0923b81 100644
> --- a/cogl/cogl-matrix-stack.h
> +++ b/cogl/cogl-matrix-stack.h
> @@ -358,7 +358,7 @@ cogl_matrix_stack_frustum (CoglMatrixStack *stack,
>   * Replaces the current matrix with a perspective matrix based on the
>   * provided values.
>   *
> - * <note>You should be careful not to have to great a @z_far / @z_near
> + * <note>You should be careful not to have too great a @z_far / @z_near
>   * ratio since that will reduce the effectiveness of depth testing
>   * since there wont be enough precision to identify the depth of
>   * objects near to each other.</note>
> @@ -528,7 +528,7 @@ cogl_is_matrix_stack (void *object);
>   * @z: (out): The destination for the z-component of the translation
>   *
>   * Determines if the only difference between two transforms is a
> - * translation and if so returns what the @x, @y, and z@ components of
> + * translation and if so returns what the @x, @y, and @z components of
>   * the translation are.
>   *
>   * If the difference between the two translations involves anything
> @@ -571,7 +571,7 @@ cogl_matrix_entry_is_identity (CoglMatrixEntry *entry);
>   * returning %TRUE if they are equal or %FALSE otherwise.
>   *
>   * <note>In many cases it is unnecessary to use this api and instead
> - * direct pointer comparisons of entries is good enough and much
> + * direct pointer comparisons of entries are good enough and much
>   * cheaper too.</note>
>   *
>   * Return value: %TRUE if @entry0 represents the same transform as
>

thanks for this doc fixes, I'll fold those back into my patch before
landing.

thanks,
- Robert


>
> Regards,
> - Neil
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> _______________________________________________
> Cogl mailing list
> Cogl at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/cogl
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/cogl/attachments/20121129/ff8e0bc7/attachment-0001.html>