dankamongmen / notcurses

blingful character graphics/TUI library. definitely not curses.
https://nick-black.com/dankwiki/index.php/Notcurses
Other
3.48k stars 112 forks source link

ncblit_rgba and friends don't return the number of pixels blitted (as promissed)? #2638

Open OBenjaminT opened 2 years ago

OBenjaminT commented 2 years ago

In USAGE.md and import/notcurses.h the comment claims that the number of pixels that are blitted are returned:

// Blit a flat array 'data' of RGBA 32-bit values to the ncplane 'vopts->n',
// which mustn't be NULL. the blit begins at 'vopts->y' and 'vopts->x' relative
// to the specified plane. Each source row ought occupy 'linesize' bytes (this
// might be greater than 'vopts->lenx' * 4 due to padding or partial blits). A
// subregion of the input can be specified with the 'begy'x'begx' and
// 'leny'x'lenx' fields from 'vopts'. ***Returns the number of pixels blitted,*** or
// -1 on error.
int ncblit_rgba(const void* data, int linesize,
                const struct ncvisual_options* vopts);

(emphasis mine)

Yet the source only returns -1 or 0?

int ncblit_rgba(const void* data, int linesize, const struct ncvisual_options* vopts){
  if(vopts->leny <= 0 || vopts->lenx <= 0){
    logerror("invalid lengths %u %u", vopts->leny, vopts->lenx);
    return -1; // RETURN -1
  }
  if(vopts->n == NULL){
    logerror("prohibited null plane");
    return -1; // RETURN -1
  }
  struct ncvisual* ncv = ncvisual_from_rgba(data, vopts->leny, linesize, vopts->lenx);
  if(ncv == NULL){
    return -1; // RETURN -1
  }
  if(ncvisual_blit(ncplane_notcurses(vopts->n), ncv, vopts) == NULL){
    ncvisual_destroy(ncv);
    return -1; // RETURN -1
  }
  ncvisual_destroy(ncv);
  return 0; // RETURN 0
}
dankamongmen commented 2 years ago

indeed, this is a bug--good eyes! the question is whether it's a bug in documentation or code. anyone who's been relying on the number of pixels being returned has been broken, and it's possible someone was (probably mistakenly) relying on the 0 return...

i think most everything else blittish returns the pixel count, so that's probably the way to go.

OBenjaminT commented 2 years ago

Yea, I'm creating a zig wrapper and was checking if I could safely cast to u32/u64 and suddenly realised I could just cast to bool... (I'm actually spending a lot of time trying to see if I can make the types in the wrapper tighter than the c types and exploring the code to double check if I'm not limiting myself. Which includes trying to find out what could cause errors and describing them better than return -1).

I don't know what you mean by "blittish" but ncblit_rgb_packed, ncblit_rgb_loose, and ncblit_bgrx all return either -1 or the result of ncblit_rgba (the buggy one). Their doc comments refer you to ncblit_rgba's so I assume they're faulty by association?

dankamongmen commented 2 years ago

Yea, I'm creating a zig wrapper and was checking if I could safely cast to u32/u64 and suddenly realised I could just cast to bool... (I'm actually spending a lot of time trying to see if I can make the types in the wrapper tighter than the c types and exploring the code to double check if I'm not limiting myself).

sweet. do know that someone else has already worked on this:

| Zig | Jakub Dundalek | dundalek/notcurses-zig-example |

I don't know what you mean by "blittish" but ncblit_rgb_packed, ncblit_rgb_loose, and ncblit_bgrx all return either -1 or the result of ncblit_rgba (the buggy one). Their doc comments refer you to ncblit_rgba's so I assume they're faulty by association?

yep, i'll fix this there and the correct behavior will propagate out.