[m-rev.] for review: mercury_opengl changes

[Julien Fischer]

On Tue, 5 Dec 2006, Peter Wang wrote:

> Julien, what do you think?

Thoughts.

> extras/graphics/mercury_opengl/mogl.m:
> 	Add bindings for glTexImage1D, glTexImage2D, glTexImage3D,
> 	glCopyTexImage1D, glCopyTexImage2D.
>
> 	Move the pixel_data type into the interface.
>
> 	Import mogl.type_tables.
>
> extras/graphics/mercury_opengl/mogl.type_tables.m:
> 	New module.  This is intended to expose some of the <type>_to_int and
> 	<type>_flags[] tables.  Modules other than mogl can import this module
> 	to convert mogl types to their equivalent OpenGL constants.

Is this a particularly useful thing to be able to do?

(Actually, I was hoping to use the foreign_export_enum stuff I've been 
working on to eliminate the need for most of the <type>_to_int predicates).

> 	Add pixel_format and pixel_type conversion tables.
>
> 	Move texture_format conversion tables from mogl to here.
>

Which version of the OpenGL spec are these changes targeted at?

> Index: mogl.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/extras/graphics/mercury_opengl/mogl.m,v
> retrieving revision 1.22
> diff -u -r1.22 mogl.m
> --- mogl.m	31 Aug 2006 11:09:50 -0000	1.22
> +++ mogl.m	5 Dec 2006 04:22:03 -0000
> @@ -32,6 +32,8 @@
> :- module mogl.
> :- interface.
>
> +:- include_module mogl.type_tables.
> +
> :- import_module bool.
> :- import_module float.
> :- import_module int.
> @@ -352,6 +354,17 @@
>     ;       luminance
>     ;       luminance_alpha.
>
> +:- inst pixel_format_non_stencil_or_depth
> +    --->    color_index
> +    ;       red
> +    ;       green
> +    ;       blue
> +    ;       alpha
> +    ;       rgb
> +    ;       rgba
> +    ;       luminance
> +    ;       luminance_alpha.
> +
> :- type pixel_type
>     --->    unsigned_byte
>     ;       bitmap
> @@ -521,6 +534,34 @@
> :- pred tex_gen(texture_coord::in, texture_gen_parameter::in,
>     io::di, io::uo) is det.
>
> +:- inst border ---> 0 ; 1.
> +
> +:- type pixel_data.
> +:- pragma foreign_type("C", pixel_data, "const GLvoid *").

I suggest hiding the foreign_type decl in it's own little interface
section, rather than including it in the "public" interface.

What's responsible for managing the memory required by the texture
data?  (It was never entirely clear to me whether textures are
copied to the server or if the reside on the client side.  There's
no problem for the former; there may be for the latter, e.g.
if the memory was allocated by the garbage collector and we no longer
maintain a reference to it.)

> +:- pred tex_image_1d(texture_target::in(texture_2d), int::in,
> +    texture_format::in, int::in, int::in(border),
> +    pixel_format::in(pixel_format_non_stencil_or_depth),
> +    pixel_type::in, pixel_data::in, io::di, io::uo) is det.
> +
> +:- pred tex_image_2d(texture_target::in(texture_2d), int::in,
> +    texture_format::in, int::in, int::in, int::in(border),
> +    pixel_format::in(pixel_format_non_stencil_or_depth),
> +    pixel_type::in, pixel_data::in, io::di, io::uo) is det.
> +
> +:- pred tex_image_3d(texture_target::in(texture_2d), int::in,
> +    texture_format::in, int::in, int::in, int::in, int::in(border),
> +    pixel_format::in(pixel_format_non_stencil_or_depth),
> +    pixel_type::in, pixel_data::in, io::di, io::uo) is det.
> +
> +:- pred copy_tex_image_1d(texture_target::in(bound(texture_1d)), int::in,
> +    texture_format::in, int::in, int::in, int::in, int::in(border),
> +    io::di, io::uo) is det.
> +
> +:- pred copy_tex_image_2d(texture_target::in(bound(texture_2d)), int::in,
> +    texture_format::in, int::in, int::in, int::in, int::in, int::in(border),
> +    io::di, io::uo) is det.
> +
> %------------------------------------------------------------------------------%
> %
> % Fog
> @@ -1159,6 +1200,8 @@
>
> :- implementation.
>
> +:- import_module mogl.type_tables.
> +
> :- import_module exception.
> :- import_module require.
>
> @@ -1178,6 +1221,8 @@
>     #endif
> ").
>
> +:- pragma foreign_import_module("C", mogl.type_tables).
> +
> %------------------------------------------------------------------------------%
> %
> % GL errors
> @@ -2516,9 +2561,6 @@
>         pixel_data   :: pixel_data
>     ).
>
> -:- type pixel_data.
> -:- pragma foreign_type("C", pixel_data, "const GLvoid *").
> -
> read_buffer(Buffer, !IO) :-
>     read_buffer_to_int_and_offset(Buffer, BufferFlag, Offset),
>     read_buffer_2(BufferFlag, Offset, !IO).
> @@ -2592,90 +2634,6 @@
>     extern const GLenum texture_format_flags[];
> ").

...

> Index: mogl.type_tables.m
> ===================================================================
> RCS file: mogl.type_tables.m
> diff -N mogl.type_tables.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ mogl.type_tables.m	5 Dec 2006 04:22:03 -0000
> @@ -0,0 +1,190 @@
> +%-----------------------------------------------------------------------------%
> +% vim: ft=mercury ts=4 sw=4 et
> +%-----------------------------------------------------------------------------%
> +% Copyright (C) 1997, 2003-2006 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU Library General
> +% Public License - see the file COPYING.LIB in the Mercury distribution.
> +%-----------------------------------------------------------------------------%
> +%
> +% File: mogl.m.
> +% Main authors: conway, juliensf.

The filename is incorrect there.

There should also be a comment describing the purpose of this module.

> +%------------------------------------------------------------------------------%
> +
> +:- module mogl.type_tables.
> +:- interface.
> +
> +:- func pixel_format_to_int(pixel_format) = int.
> +
> +:- func pixel_type_to_int(pixel_type) = int.
> +
> +:- pred texture_format_to_int(texture_format, int).
> +:- mode texture_format_to_int(in, out) is det.
> +%:- mode texture_format_to_int(out, in) is semidet.

Why is that mode commented out?

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------