Re: [AD] WIP compressed texture support patch

[Brandon McCaig <bamccaig@xxxxxxxxxx>]

SiegeLordEx:

On Mon, Dec 8, 2014 at 11:33 PM, SiegeLordEx <slabode@xxxxxxxxxx> wrote:
> With this talk about 5.2, it'd be nice not to write ourselves
> into a corner with this question... I think it might be as
> simple as simply removing the al_use_tex_matrix from shaders or
> w/e its called and making the use of the texture matrix
> mandatory.
>
> Another issue is that compression/decompression on Windows
> requires the D3DX library. I made it a hard requirement for
> this feature (it still loads the necessary function dynamically
> though).
>
> Anyway, I'd prefer if somebody looked over all this and checked
> if anything looks crazy. Don't worry about the formatting, etc,
> I'll fix it later. I'll also write documentation and tests.
> Also, I don't know if this works at all on Android/iOS/OSX, so
> it'd be nice if this was tested too (at least to see if it
> compiles). I only implemented DXT1/3/5 formats, but there are
> others... if the ones I implemented don't work, then perhaps
> some others will. The framework should be good enough to handle
> them (although the OpenGL flipping bit will need to be
> implemented for them).
>
> The patch is attached, and you can also look at this:
> https://github.com/SiegeLord/allegro5/compare/compression .

That's a, uh, sizable patch. Does this stuff need to be
explicitly turned on to have any affect? I'd say that's the first
thing. In terms of just getting a release out and making noise
within the community, so what if the patch doesn't work perfectly
or could use some work. It's probably too big to get it perfect.
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. Just mark this
feature as "experimental" until such a date that it has been
stress tested with real world code and perhaps you'd have a
chance to tidy it up.

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

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). ;)

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

# Create backup ref for easy recovery and reference.
git branch orig-mybranch;

# Reset the branch and index back to before your branch diverged.
# (This is a context-sensitive command... HEAD~n as appropriate)
git reset HEAD~$(git log --oneline @{u}.. | wc -l);

# while dirty tree...
    git diff;
    *review review review*
    git add -p FILE...;
    *load logically ordered changes into index*
    git commit -m '<Sensible patch that you can understand \
or that I explain here that builds up to full idea.>';
# endwhile

# Sanity check.
git diff orig-mybranch;

# If necessary.
git push -f myremote mybranch;

Does the Allegro project use patch series' (e.g., ordered set of
patches for review, effectively equivalent to commits)? That
might make this more understandable. Instead of one large patch,
format-patch the branch (after cleaning up as above since your
current history is kind of h4xy).

Knowing how to do this takes practice (though you're a smart guy
so you may well have that practice already), but I generally try
to work my way from the backend towards the frontend. That
*usually* helps you to make sense of it yourself. Honestly, after
you've hacked away at a problem it's quite hard to simplify it
into distinct patches, especially if you're tiring of the work or
under the gun to move on. It can be done if you have the time
though.

I digress. I'm not normally participating in Allegro's
development so feel free to tell me to `goto PARK'. As I said
above, if the code is relatively non-invasive I don't see any
reason to delay "3.2" or whatever the next release is. Take
advantage of the little bit of attention that has been generated
on the forums and try to stir up trouble. You can't really make
things worse with version control. You can only liven up the
conversation (or make more productive team members ragequit, but
they usually put up a fight first).

Whoever does the release just make bold/"strong" notes wherever
it is published that the project has decided to release earlier
than past cycles to fuel the project and that developers
requiring more stability and not requiring bleeding edge features
(listed of course) should buffer themselves the latest releases
until some of the bugs have been worked out.

I'm only 353/3293 lines into the patch and I can't stay up any
later because j0rb is taking it out of me this week. :-[ If this
post is not unwelcome I can attempt to continue tomorrow (or
whenever I next have the time).

 >:(

Regards,


-- 
Brandon McCaig <bamccaig@xxxxxxxxxx> <bamccaig@xxxxxxxxxx>
Castopulence Software <https://www.castopulence.org/>
Blog <http://www.bambams.ca/>
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'


PARK: fly_kite(kite);

Attachment: signature.asc
Description: Digital signature