hey @Sean i have done the following things for in-DB support for vol
(deleted)
should i submit it as a patch?
or do something more and then submit it as a patch?
things i have done till now for in-DB support -
tasks.txt
holy moly!
what?
just that you've been making progress, that's fantastic!
did i do something wrong?
and looks like you're on the right track
i wouldn't post it to sf.net as a patch until it can be committed
ok
but you can post it here and can see if there's any feedback or issues
i am right now building it with target all
since you implemented the prompt for the filename, that will affect the ability to merge it (e.g., if it prompts now and then we post a release, that'd be a problem of course since people would expect if its prompting it must be doing that thing
but yeah, this is fantastic
thats why i was asking if i should post it or not right now
yeah, just share it here if you like
i saw the release thread
or if you want to extract the portions that can be committed, you could submit those
ok. let the build finish. then i will post it here
i.e., not the typein.c changes
ok
(deleted)
this include most of the changes i did for in-db support
except for the typein.c changes
this was all i could do in tonight's time
/me looks
good night
okay, see you in your morning ;)
tell me if it needs more changes in the other files
I just looked through the patch file and it looks spot-on to me.
no changes necessary to the ones you made -- it of course depends what you do with those changes, but looks like you're following what ebm is doing.
notice that dsp is slightly different (and more simple in some ways) as i t uses vls strings and doesn't have as much type toggling
I am going from ebm because I have made one and saw how it works
So i want vol to work the same way
it's fine, the differences are subtle
both dsp and ebm essentially do the same thing. main difference is little things like ebm stores the object name in a name field that's a char[] whereas dsp stores it in a bu_vls string.
Ok
so you'll see differences accessing the object name
ebm can just use the name string, whereas dsp has to call bu_vls_cstr() or bu_vls_addr() to get the name string. of course, ebm has to have overflow/buffer checks though, and dsp does not -- dsp memory is semiautomatically managed.
when it comes to accessing the data, you'll want to follow what ebm is doing anyway
so it looks like this can be committed. i haven't tested it to make sure it compiles, but if you can test that, you could submit it to sf.net as your second patch!
it will not build actually
a change i made in include/rt/geom.h was changing the char file
variable to char name
.
because of which a lot of problems will crop up during build in typein.c
i will submit it later to sf.net after making some more necessary changes
so i tested the 'f' part of the question and it still works
only part that is left is the 'o' part
@Sean tasks.txt
done upto this. now the only thing that is left is build it and test it if it works
(deleted)
(deleted)
its building successfully but not working
now its printing the following stuff:
ERROR: Binary object 'dbobj' has invalid data (expected type 4, found 4).
Expecting 160000 8-bit unsigned char (nuc) integer data values.
Encountered 480000 unsigned 8-bit ints
Couldn't find the associated file/object dbobj
ERROR: Binary object 'dbobj' has invalid data (expected type 4, found 4).
Expecting 160000 8-bit unsigned char (nuc) integer data values.
Encountered 480000 unsigned 8-bit ints
Couldn't find the associated file/object dbobj
ERROR: Binary object 'dbobj' has invalid data (expected type 4, found 4).
Expecting 160000 8-bit unsigned char (nuc) integer data values.
Encountered 480000 unsigned 8-bit ints
Couldn't find the associated file/object dbobj
this is the patch patch.patch
Couldn't find the associated file/object dbobj
i added this line for debugging purposes
its now working perfectly
but it is not showing up on the graphic window
this is the things i have done in there
and this is the patch after fixing the problems patch.patch
That's great. Did you post the patch of the bits that were working to sf.net without the typein user prompting?
actually i implemented it fully
except for the visual part which i am trying to fix
seriously? that's awesome!
file part is working properly
only object part's drawing has to be fixed
its also building properly
well building is the first and typically easiest step. lots of ways for something to compile and still be quite wrong. :)
so you got the binunif aspect sorted out with the right data types?
bw (i.e., 8-bit unsigned char) is the native format for vol slices and it will need to read either multiple bw or one bw with multiple layers (you probably fed the latter in your test?)
Yea
I fed one bw with multiple layers
Should I do something else instead of this?
No, I think that's fine @Sumagna Das
I mean it might be nice if the user didn't have to combine them all, that gcv did it for them, but the internal guts in src/librt/primitives/vol should just be one data object
we'll likely change / extend that data object later with a proper "image" object instead of a binunif, but that's future-talk
maybe your third set of commits ;)
going to submit the patch soon?
Yea
After fixing the drawing issue
static int
get_obj_data(struct rt_vol_internal *vip, const struct db_i *dbip)
{
struct rt_binunif_internal *bip;
int ret;
int nbytes;
BU_ALLOC(vip->bip, struct rt_db_internal);
ret = rt_retrieve_binunif(vip->bip, dbip, vip->name);
if (ret)
return -1;
if (RT_G_DEBUG & RT_DEBUG_HF) {
bu_log("db_internal magic: 0x%08x major: %d minor: %d\n",
vip->bip->idb_magic,
vip->bip->idb_major_type,
vip->bip->idb_minor_type);
}
bip = (struct rt_binunif_internal *)vip->bip->idb_ptr;
if (RT_G_DEBUG & RT_DEBUG_HF)
bu_log("binunif magic: 0x%08x type: %d count:%zu data[0]:%u\n",
bip->magic, bip->type, bip->count, bip->u.uint8[0]);
if (bip->type != DB5_MINORTYPE_BINU_8BITINT_U
|| (size_t)bip->count != (size_t)(vip->xdim*vip->ydim*vip->zdim))
{
size_t i = 0;
size_t size;
// size_t ret = 0;
struct bu_vls binudesc = BU_VLS_INIT_ZERO;
rt_binunif_describe(&binudesc, vip->bip, 0, dbip->dbi_base2local);
/* skip the first title line*/
size = bu_vls_strlen(&binudesc);
while (size > 0 && i < size && bu_vls_cstr(&binudesc)[0] != '\n') {
bu_vls_nibble(&binudesc, 1);
}
if (bu_vls_cstr(&binudesc)[0] == '\n')
bu_vls_nibble(&binudesc, 1);
bu_log("ERROR: Binary object '%s' has invalid data (expected type %d, found %d).\n"
" Expecting %zu 8-bit unsigned char (nuc) integer data values.\n"
" Encountered %s\n",
vip->name,
DB5_MINORTYPE_BINU_8BITINT_U,
bip->type,
(size_t)(vip->xdim*vip->ydim*vip->zdim),
bu_vls_cstr(&binudesc));
return -2;
}
nbytes = (vip->xdim+VOL_XWIDEN*2)*(vip->ydim+VOL_YWIDEN*2)*(vip->zdim+VOL_ZWIDEN*2);
if (!vip->buf) {
size_t x, y, z;
unsigned char* cp;
/* Prevent a multi-processor race */
bu_semaphore_acquire(RT_SEM_MODEL);
if (vip->buf) {
/* someone else beat us, nothing more to do */
bu_semaphore_release(RT_SEM_MODEL);
return 0;
}
vip->buf = (unsigned char *)bu_calloc(1, nbytes, "rt_vol_import4 bitmap");
bu_semaphore_release(RT_SEM_MODEL);
/* Because of in-memory padding, read each scanline separately */
cp = (unsigned char *)bip->u.uint8;
for (z = 0; z < vip->zdim; z++) {
for (x = 0; x < vip->xdim; x++) {
/* bit() addresses into buf */
memcpy(vol_bit(vip, x, 0), cp, vip->ydim*vip->xdim);
}
cp += vip->xdim;
}
}
return 0;
}
this is the function for taking binary object as a data source for vol
can you tell me whats wrong
After fixing the drawing issue
I don't see anything obvoius
oh, looks like the memcpy is wrong in the middle of that loop
copying the same cp over and over
if it's doing what I think you're intending, you may need to pull the cp += up into the loop
Sean said:
if it's doing what I think you're intending, you may need to pull the cp += up into the loop
its in the correct loop?
but instead of at the end. it should be at first of the same loop?
think about what those loops are doing
the inner loop is copying a row of pixels
outer one is for each layer, and the inner one is for each pixel
i added one more loop for y dimensions but it slowed down the making of the vol very much
Sean said:
the inner loop is copying a row of pixels
yea
not each pixel, it's each row.. so that loop basically copies one image
yea
so as it iterates, it's copying the bytes from cp into vip
yea
well it's copying the same cp every iteration of the loop, and the loop is only incrementing x, which is the index into vip's array (via vol_bit())
but not incrementing cp because it's outside the loop
bring it in, and it should be copying the bytes correctly.
i think something else be added to cp
i just dont know what should be added
huh?
what else? you loop is right there...
zdim is actually adding 3 each time
there's only one line of code in the loop and the for condition that could possibly increment it
Sumagna Das said:
zdim is actually adding 3 each time
and is that correct?
i dont think it is correct
the inner loop is copying one layer i.e., one image out of the combined bw
Sean said:
Sumagna Das said:
zdim is actually adding 3 each time
and is that correct?
zdim is actually adding 3 for me because i am using the same picture to test it
adding 3 to what though
cp
and why would you have it do that?
i dont know actually. i just was thinking about changing it but forgot
let me test
memcpy(vol_bit(vip, x, 0), cp, vip->ydim*vip->xdim);
vip->ydim*vip->xdim
this is correct for the number of bytes right?
is it?
what is xdim?
the x dimensions of the file i.e. the width of the image
and ydim?
"dimensions of the file" <-- technically incorrect
y dimensions of the file i.e. the height of the image
also technically incorrect
not file
i meant the image file
what is the input?
input to the binary object or input to the vol?
they are one in the same, no?
actually yea
the bw file of an image is inputted to the binary object and the binary object is inputted to the vol
right
so think of the bw file as a big array of bytes, because it is that
how many bytes is it?
400 * 400 *3(for me)
x dimension * y dimensions *z dimensions
which is how many bytes?
480000
great
so at the end of the day, this function you've written -- this get_obj_data() function -- is making a copy of the data putting it into its vip structure
it should do that
so you sort of copied what you saw ebm was doing , right?
but the code i have written is not doing that correctly
it's not, but it's important to understand what you're trying to do first
Sean said:
so you sort of copied what you saw ebm was doing , right?
somewhat. i also saw how the vol is being made from a file in vol_from_file()
function
ah, excellent
but as it is reading a file directly, i cant follow it much
okay, no problem. I don't think you need to.
so the first question you should be asking is why are we even making a copy of the data at all given we could just access the data directly in (unsigned char *)bip->u.uint8;
that also can be done.
then extra memory wont be used
but that's a question for another day -- we see that ebm and dsp are making copies, so just for sake of argument lets assume there's some reason why
so assuming we need to copy the data out of bip->u.uint8, where are we copying it into?
from cp to vip?
don't abstract it, what's the actual to and from arrays or pointers?
cp is a pointer to the actual "from" array
from a void pointer to another void pointer?
no, those are types -- i mean what is the actual container, the actual array of bytes?
the "from" container is what cp is pointing at -- the "from" is bip->u.uint8
where are we copying "to" ?
the "to" pointer is pointing to vip->buf
excellent
how much are we copying?
i changed it right now so its copying vip->xdim number of bytes(or 400 for me)
yes, to copy by line, that is one fix
its in the inner loop btw-> for (y = 0; y < vip->ydim; y++)
yep
but if you continue with the question, you'll see this all probably simplifies greatly
how many bytes need to get copied from bip's array into vip's array?
overall or each time?
overall
ie.., what's this functions ultimate job
for me its probably about 480000 bytes
why "about 480000" ?
oh sorry it is 480000 bytes
better
so why not just do that?
copy in whole?
and not iterate?
memcpy() from bip's array to vip's array, that many bytes
one line of code
no iterations?
it's worth trying
wait a minute then
its building
there's a comment making some claim about alignment, but I think that was someone copying code from somewhere else without fully understanding the code they were copying.
and it got used again in some place (probably dsp) and again in another (ebm) and you've copied it again .. but the underlying premise that it needs to be iterated doesn't look valid to me because bip is just a big array.
arrays can be copied to arrays without iteration.
actually yea
i copied but i also doubted why the array was being copied while iterating
there's nothing wrong with iterating
other than it's just unnecessary ;)
let memcpy iterate
yea but it got messy
is this correct -> memcpy(vip->buf, cp, vip->xdim*vip->ydim*vip->zdim);
?
looks like about what I had in mind
nothing appears in the graphic window
still
well that's not to say there aren't other issues
thats the main issue right now
well that's a big assumption :)
it's the issue you can see is an issue (or can't see rather, hah)
Sean said:
well that's a big assumption :smile:
yea
everything is going correctly but its still not showing up
okay, so first there are at least two issues. even if our memcpy is no good for some reason, it should at last just be messing up cell values, not affecting what is displayed
when you draw something, that ends up calling into the *_plot() function
ooh
so maybe start debugging from there
uh oh, and we do have at least another problem I think
just reading the file, I see this comment:
/*
* Regular bit addressing is used: (0..W-1, 0..N-1),
* but the bitmap is stored with two cells of zeros all around,
* so permissible subscripts run (-2..W+1, -2..N+1).
* This eliminates special-case code for the boundary conditions.
*/
which file?
that's in vol.c
thats why it seems familiar
i saw this but didnt think it was going to matter
if it's a true statement, that changes everything
why?
think about it... a vol is a stack of bitmap images.. if every image is being wrapped in zeros, that completely changes the bytes and copying patterns needed
so consider an example:
say our vol is a 2x2x3:
01
23
45
67
89
01
three little 2x2 images
those byte values in a bw file would be: "012345678901"
no newlines
at least, if we consider these byte indices as array indicies, that's what it'd look like. bw files are actually bottom to top, so it'd actually be "230167450189"
newlines??
what newlines???
i mean its not taking them as individual lines
its taking the file as a whole
I think you're completely missing what I'm talking about if you're thinking about newlines :)
we're talking about vol data, bw data
they are unsigned 8-bit bytes
binary data
raw 0-255 values
that was a random thought. sorry for confusing.
what I "drew" was simply a visual depiction of bw values to help understand the ordering and quantity of bytes
so say you have a 2x2 bw image
it's looks sort of like this from an indexing perspective:
01
23
let say those numbers are the actual image values, just for simplicity
so on disk and in memory, the bytes will be "2301"
does that make sense?
yea
I can literally put that into code: unsigned char my_bw_image[4] = {2, 3, 0, 1}; ... that's a bw file in code
so say I have two more bw images like I showed above also. and we join them into one .bw data file using copy or cat or whatever
the bytes are simply "230167450189" for those three layers. understand that?
yea
so then if I read that bw data into a binunif array called bip, what's the value of bip.u.uint8[6] ?
4?
good
yes
okay, so now here's the stinker
that comment is saying that while the bip might be "230167450189" ... it's going to pad each of the images with zeros
like "023010...."
so instead of "2301" for the first layer, those bytes will be...
000000
000000
000100
002300
000000
000000
oooh
well, I'm assuming it's padding vertically, it may only pad horizontally like:
000100
002300
either way.. that changes the layer data in the vol to probably being "000000000000002300000100000000000000" for the first layer and so on for the other two
so what should be done?
well we have to confirm what is going on in the file read function like how you'd started and understand what it's doing (especially now that we know there may be padding being added). just keep in mind that the comment could also be totally wrong or misleading and there may be no padding, but it totally changes things if there is so we have to look and find out.
yea
we know where the data is ending up, vip->buf
so just have to look at anything being passed vip or a pointer to vip
yea but i saw there was already an unsigned char ** in vip called map
where the data was being stored for file
so why'd you say vip->buf then? :)
i saw that right now
there is no vip->buf
there is one
there's not in my code
Sumagna Das said:
yea but i saw there was already an unsigned char ** in vip called map
i am just saying can i store in this one?
saying or asking? :)
asking
you are making these decisions
with these changes.
we know we need to copy the bw data into the vol, at least that was an assumption we decided
so yeah, if the file reader is storing all the bw data into vip->map, then I'm not sure why we'd possibly want to store into anything else
if you did store into something else, that would explain why nothing plots. i mean if you added a vip->buf and there's a vip->map and the vol_plot() function is looking at vip->map and you copied into vip->buf... of course it's not going to find it.
that was my fault
so yeah, store into vip->map using a method similar if not identical to vol_from_file, but instead of fread() to copy bytes from file to memory, it'll be memcpy() to copy bytes from binunif to memory
right
use the same macros because they're the ones responsible for padding
yeah, here's the key:
/* Get bit map from .bw(5) file */
nbytes = (xdim+VOL_XWIDEN*2)*
(ydim+VOL_YWIDEN*2)*
(zdim+VOL_ZWIDEN*2);
*map = (unsigned char *)bu_calloc(1, nbytes, "vol_import4 bitmap");
see nbytes? that's not xdim * ydim * zdim...
i have the nbytes set up
but do you understand now what that's doing?
now i understand what the macros were for
vol_from_file() allocated an array (zero-initialized via calloc) with (xdim+4) * (ydim+4) * (zdim+4)
the macros are making it skip into the array at the right position given it copied
01
23
into memory that looks like "000000000000002300000100000000000000", so the macros help jump index correctly into it
alright, my turn to rest. good luck and catch up with you in a few hours ;)
woohoo!! Screenshot-from-2020-09-16-11-27-29.png
created a vol from binary object
(deleted)
hey @Sean i submitted the patch containing all the changes
see if it is okay or not
if any changes are needed, tell me
did you check my patch?
@Sumagna Das I did check your patch and it looks good to me. that said, I haven't finished testing it but should be done soon.
did it work?
@Sean What's next?
@Sumagna Das so next is back in the plugin, no?
did it work?
oh, actually next would be to update the wdb interface
to include the obj changes?
I still haven't finished testing it. A bug came up in testing (unrelated to your changes) that I had to debug and fix. just finished up with that.
Ok
I should have it checked out here in a few hours while you're sleeping ;)
my reading of the code looked like it was good
I didn't catch any blatant issues
comments might be less though
Sean said:
oh, actually next would be to update the wdb interface
what should be updated?
what do i have to update though?
@Sean what should i update in the wdb interface?
@Sumagna Das you maybe already did, but the mk_vol() function saved the name into the ->file field of the vol, and I believe you renamed it? Assuming you did, the function could use an additional parameter to indicate whether that is auto, file, or object
Yea I changed that because it was causing compilation errors
I can add a parameter like datasrc corresponding to the actual variable in rt_vol_internal
Sean said:
Sumagna Das you maybe already did, but the mk_vol() function saved the name into the ->file field of the vol, and I believe you renamed it? Assuming you did, the function could use an additional parameter to indicate whether that is auto, file, or object
what will the datasrc parameter do anyways?
the same thing it does during the 'in' command prompting, it's the field you added to the struct that says whether the data is in an object or a file or auto
That means it will tell the function whether a binary object has been passed or a file name has been passed.
@Sean should i change the const char *
parameter for the file name to a void *
and it will be used according to the (new)datasrc
parameter? what do you think?
I actually meant to say that if the datasrc
tells the function that it has been passed an object then the void *
parameter will be casted to rt_binunif
(or whatever the name of the the struct was) and if it tells the function that a file is going to be read then the generic pointer will be casted to char *
No, it's still just a name - it's not passed an actual binunif @Sumagna Das .. callers first invoke mk_binunif() and then mk_vol() passing the name of the binunif created and auto or obj for the type.
so char * name
will stay the same (i am talking about this parameter because i dont want to add another new parameter except for the datasrc
parameter)
correct, it's just the object or file name
then the source parameter can indicate which
so i only have to add the datasrc
parameter
correct
Sean said:
then the source parameter can indicate which
yea
Sean said:
No, it's still just a name - it's not passed an actual binunif Sumagna Das .. callers first invoke mk_binunif() and then mk_vol() passing the name of the binunif created and auto or obj for the type.
auto for the type is next after changing the mk_vol (if i can)
ah, I see that neither mk_dsp() or mk_ebm() were updated. they both appear to default to 'auto'.
actually, my recollection that they had an AUTO mode is apparently flawed also.
if I had to guess, I might guess that mk_dsp() and mk_ebm() are currently both broken because they don't set it and the code appears to treat unset/0 as an error since it's expecting 'o' or 'f'
would be nice to extend our fuzzer to call into the mk_() API....
fuzzer?
@Sean will wdb_export
handle the vol's input source according to vol->datasrc
?
@Sumagna Das wdb_export ends up calling ft_export which ends up calling rt_vol_export5() that you probably updated... but that would be the place to check whether you're serializing the datasrc field in the export5 function and deserializing it in the import5 function.
Sean said:
Sumagna Das wdb_export ends up calling ft_export which ends up calling rt_vol_export5() that you probably updated... but that would be the place to check whether you're serializing the datasrc field in the export5 function and deserializing it in the import5 function.
i updated the function which both rt_vol_import5
and rt_vol_export5
functions call so it might be less messy.
looks like your patch file is missing some changes @Sumagna Das .... include dir changes weren't included
seriously?
i might have to update the patch then
definitely
updated it already
I see it now
cleaned it up a bit also
So indentation we use is atypical these days (used to be more common)... it's 4 char indents BUT 8 char tab stops
so 8 spaces for tab?
what's your editor?
atom
I'm not as familiar with atom, but yeah I think you want to set 8 spaces for tab, and I think hard tabs, but then somewhere you want to set 4 char indentation
looks like the auto mode may work too
for mk_vol?
for atom
atom has an auto tabs indentation mode where it detects what the file is using
(deleted)
ohh
I was too... I don't understand the mk_vol question
this is for any code in brl-cad
Sean said:
looks like the auto mode may work too
i thought you were talking about auto in mk_vol
check out the "Indentation whitespace" section in HACKING for a visual diagram of the indent rules
ok
will do that
right, and that's why I said auto refers to atom indent settings
there's an auto tab setting
you might have to just try some edits to see, but first step will be to make sure you understand what it should be...
yea
I'd be a little surprised if atom can't handle the hybrid mode by default with the right settings, but this package may implement it: https://atom.io/packages/atom-smart-tabs
Sean said:
I'd be a little surprised if atom can't handle the hybrid mode by default with the right settings, but this package may implement it: https://atom.io/packages/atom-smart-tabs
thanks for that.
actually, never mind -- that's not quite what is needed
i will check it later today
that's tab indentation with alignment .. we use hybrid indentation chars
uh oh, looks like atom doesn't support it
https://discuss.atom.io/t/distiguish-between-tab-size-and-indentation-size/40197
ohh
what the moderator suggested appears to be a workaround, but a bit involved
i.e., insert spaces with tab width set to 4 but then replace all sequences of 8 chars with a tab on save
yep, this looks like the issue, auto-closed: https://github.com/atom/atom/issues/12931
I opened a new issue: https://github.com/atom/atom/issues/21371
Sean said:
I opened a new issue: https://github.com/atom/atom/issues/21371
thanks for that
so yeah, @Sumagna Das .. you're kind of in a hard place using Atom. It doesn't support the style we use so you're going to either have to deal with it manually or use another editor :(
we are planning on shifting to a different indentation style, but that's not happening yet/soon
i think i will deal with it manually right now
@Sean so should i set 4 for tab length in atom for now?
or 8
need some help with mk_binunif
@Sumagna Das I'd probably think it easiest to set it to 8 (with tabs, not spaces) and just manually fix
Ok
does the patch work?
hey @Sean does the patch work?
if yes then i will try to continue on it
hey @Sean , i am switching to vscode for now.
so are the indentation and tab space settings different in there?
i used it many days ago but didnt know much about editor so if you know please tell me
Hahaha, @Sumagna Das VScode has the same problem. :)
:lol:
It's Visual Studio (the Professional version) that supports mixed-mode. Looks like for VSCode that some of the devs want to implement it and others want to encourage projects to choose spaces vs tabs.
i am one of those devs
Don't worry about it. Just insert spaces for now. They're the easiest to fix.
i switched to vscode for some perfomance and extension issues
We're planning on switching, but then I thought we'd decided on one thing and others thought we'd decided on something else, and it was deferred
Oh
and what is the status of the Github migration
and then deferred again for gsoc, and then release, and the can keeps getting kicked as once we do switch it, it will make most patches fail to apply cleanly (or at least a significant number of them) ... so there's some desire to close the patches out first.
there's a checklist on the wiki. I had a few remaining issues that needed to be manually checked by someone other than starseeker. You helped with a couple of them, so some can be checked off.
i am checking that list but seems like nothing has changed or it hasnt progressed much because of the releases
yes, nothing progressed while release was happening
it's only been in the past week, I finished one of them and need to annotate the ones you did
(or you can)
Sean said:
(or you can)
i dont know about that
no worries
@Sean did you check the vol patch if it works? i will proceed only after fixing all the bugs in there (if there are any)
Sorry @Sean that I couldn't update the patch as I was busy with my studies. Will resume working on it after setting up my editor for BRL-CAD.
@Sean Did you see any bugs with the vol patch or anything more i need to add to the patch? I don't think i missed anything.
Last updated: Jan 09 2025 at 00:46 UTC