Re: [AD] WIP compressed texture support patch

[SiegeLordEx <slabode@xxxxxxxxxx>]

On 12/08/2014 10:22 PM, Brandon McCaig wrote:

> However, if you can be reasonably sure that it won't break
> existing code, and that the user has to request to use it, then I
> see no reason to make a fuss about including it.

This patch is a bit invasive in a few internal bits, and every time they 
are touched, there is a chance of breakage.

> SiegeLordEx's patch <compressed_wip.diff> said:
> > +   lr = al_lock_bitmap_blocked(bmp, ALLEGRO_LOCK_WRITEONLY);
> > +   ptr = lr->data;
> > +
> > +   for (ii = 0; ii < h / block_width; ii++) {
> > +      size_t pitch = (size_t)(w / block_width * block_size);
> > +      num_read = al_fread(f, ptr, pitch);
> > +      if (num_read != pitch) {
> > +         ALLEGRO_ERROR("DDS file too short.\n");
> > +         goto FAIL;
> > +      }
> > +      ptr += lr->pitch;
> > +   }
>
> Bare in mind that I'm completely unfamiliar with Allegro's source
> code, and graphics programming for that matter, so I'm just
> glancing over this with programmer eyes, but I'm not seeing how
> lr->pitch is corresponding to the for-loop's pitch. Is that
> intended as a sanity check to make sure the maths is correct, or
> is it safe to assume that the "pitch" is known by the format
> already and calculating it here is redundant or potentially
> erroneous? It appears to be a function of the image size, which
> is known by the bitmap already, and the block dimensions, which
> appear to be defined by the format..

These two pitches are often different (often just by their sign). The 
`size_t pitch` is indeed defined by the on-disk format, but the 
`lr->pitch` depends on which backend is used, and, e.g. with textures 
with non-power of 2 dimensions are supported.

> I realize you said you would clean up formatting so I'm not sure
> if this falls into that category or not... While I'm nitpicking,
> I think that 'ptr' is a pretty poor name in a function this size
> (I saw enough of that at j0rb today tyvm). ;)

Fair enough.

> SiegeLordEx's patch <compressed_wip.diff> said:
> > -            al_clear_to_color(al_map_rgb_f(0, 0, 0));
> > +            al_clear_to_color(al_map_rgb_f(0.1, 0, 0.1));
>
> That seems to come out of nowhere. ;) I'd suggest resetting your
> branch and recommitting changes in logical steps to make it more
> understandable so that you can explain this sort of change. E.g.:

Good catch, that's just for debugging purposes. I was indeed considering 
doing some history rewriting before final submission (if I can separate 
it into bits that compile independently). The reason I'm sending this 
out in this messy form is because I can't touch the code without 
contaminating it with my employer's ownership (well, I guess I could 
merely re-arrange things without that coming into play), which ruins the 
ability to collaborate. I'd rather get as many critical design comments 
in before I do anything drastic.

-SL