EBM · brlcad · Zulip Chat Archive

Hey @Daniel Rossberg , for now, could you help me work through the EBM task? I'm still not sure how I should be reading bip and then exporting.

Jeffrey Liu (Dec 23 2019 at 17:50):

/* Get bit map from .bw(5) file */
mp = bu_open_mapped_file_with_path(dbip->dbi_filepath, eip->file, "ebm");
if (!mp) {
    bu_log("rt_ebm_import4() unable to open '%s'\n", eip->file);
    bu_free((char *)eip, "rt_ebm_import4: eip");
fail:
    ip->idb_type = ID_NULL;
    ip->idb_ptr = (void *)NULL;
    return -1;
}
eip->mp = mp;
if (mp->buflen < (size_t)(eip->xdim*eip->ydim)) {
bu_log("rt_ebm_import4() file '%s' is too short %zu < %u\n",
        eip->file, mp->buflen, eip->xdim*eip->ydim);
goto fail;
}

But for my implementation, I think that this should only run when datasrc is specified to be 'f'. Otherwise, something else should run instead to read the object, right?

Jeffrey Liu (Dec 23 2019 at 17:52):

Jeffrey Liu (Dec 24 2019 at 06:33):

Hey @Sean, in regards to the fix you mentioned on my task, would this be correct?

const struct bu_structparse rt_ebm_parse[] = {
    {"%s",  RT_EBM_NAME_LEN, "file", bu_offsetof(struct rt_ebm_internal, file), BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"%s",  RT_EBM_NAME_LEN, "name", bu_offsetof(struct rt_ebm_internal, file), BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"%c",  1, "src",   RT_EBM_O(datasrc),  BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"%d",  1, "w",     RT_EBM_O(xdim),     BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"%d",  1, "n",     RT_EBM_O(ydim),     BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"%f",  1, "d",     RT_EBM_O(tallness), BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"%f",  16, "mat", bu_offsetof(struct rt_ebm_internal, mat), BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
    {"",    0, (char *)0, 0,            BU_STRUCTPARSE_FUNC_NULL, NULL, NULL }
};

Jeffrey Liu (Dec 24 2019 at 06:34):

I took a look at the DSP example and noticed that there was something called hook_file in place of BU_STRUCTPARSE_FUNC_NULL, but I couldn't seem to find it in EBM

Jeffrey Liu (Dec 24 2019 at 06:37):

Also, is it alright if my submission patch for the second task also includes the patches from the first?

Sean (Dec 24 2019 at 06:39):

hey @Jeffrey Liu sorry for missing all your questions since you started that hard task... just a bit under the weather with a cold and it's taken up some of my energy

Jeffrey Liu (Dec 24 2019 at 06:40):

No worries! I managed to figure it out and I understand that the mentors are obviously busy outside of GCI. I hope you're feeling better now?

Sean (Dec 24 2019 at 06:40):

if the first patch isn't applied yet, then yes but then you can also just post it as an update to the same patch

Sean (Dec 24 2019 at 06:42):

just keep an eye out, run svn up and check svn log to see if/when it gets applied. I was going to tonight, but alas cough cough I'm still feeling pretty crappy and need to get some extra sleep

Jeffrey Liu (Dec 24 2019 at 06:44):

Ah it's alright, I don't want to keep you up if you're sick - so would it be fine if I just reran svn diff > ... when applying my changes, even if the previous ones are still there?

Sean (Dec 24 2019 at 06:44):

Sean (Dec 24 2019 at 06:45):

just keep an eye out for conflicts as you'd have to resolve those before running diff

Jeffrey Liu (Dec 24 2019 at 06:46):

Jeffrey Liu (Dec 24 2019 at 06:47):

So regarding the other question I just asked, is that all I should change, or am I missing something?

Sean (Dec 24 2019 at 06:54):

Sumagna Das (Dec 24 2019 at 06:55):

Sean (Dec 24 2019 at 06:55):

only problem is you're using the wrong whitespace rules (presumably in MSVC or visual code)

Sean (Dec 24 2019 at 06:55):

Sumagna Das (Dec 24 2019 at 06:55):

Jeffrey Liu (Dec 24 2019 at 06:56):

Shoot, I can take a look at it again if there's something wrong. Was it something specific or just in general?

Sean (Dec 24 2019 at 06:57):

Jeffrey Liu (Dec 24 2019 at 06:58):

Sean (Dec 24 2019 at 06:58):

Jeffrey Liu (Dec 24 2019 at 07:00):

Great! I'll update now and then continue working. It seems like I was a bit behind on the revisions before though, and some CMake files got updated. Is it necessary for me to reconfigure everything and then recompile?

Jeffrey Liu (Dec 24 2019 at 07:22):

Hey @Sean, sorry to bother you again but I had one last question when I was reading through your comment. Would it be enough to add two entries to rt_ebm_parse like this,

{"%s",  RT_EBM_NAME_LEN, "file", bu_offsetof(struct rt_ebm_internal, file), BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },
{"%s",  RT_EBM_NAME_LEN, "name", bu_offsetof(struct rt_ebm_internal, file), BU_STRUCTPARSE_FUNC_NULL, NULL, NULL },

Sean (Dec 24 2019 at 07:23):

Sean (Dec 24 2019 at 07:24):

the field should get renamed, but if you do that, then bu_offsetof() has to be updated

Jeffrey Liu (Dec 24 2019 at 07:25):

Oh right, my bad for not noticing that. If I recall correctly, there's a bu function that can be used to get the offset of a string. Should I be using that?

Sean (Dec 24 2019 at 07:25):

you are correct though in that you do want to structparse entries, one for "file" and one for "name" as those are strings that get serialized out to the .g file, but both will point to the renamed 'name' field

Sean (Dec 24 2019 at 07:26):

what that {...} line is doing is initializing an array with a bunch of field that help with parsing C structs automatically

Sean (Dec 24 2019 at 07:28):

one of the columns there is bu_offset(...) .. what that is actually doing is reporting how many bytes into a "struct rt_ebm_internal" one can find a field named "file"

Sean (Dec 24 2019 at 07:28):

Jeffrey Liu (Dec 24 2019 at 07:28):

Ah I see, so the only change that needs to be made is to change the second line's file to name again?

Sean (Dec 24 2019 at 07:30):

no, you want a "file" line and a "name" line, but the byte offset for both will be to a name field after you rename the field in rt_ebm_internal

Jeffrey Liu (Dec 24 2019 at 07:31):

oh wait, so both should be changed to name? Sorry I didn't really understand what the second argument was doing at first, as it wasn't a traditional string

Sean (Dec 24 2019 at 07:32):

Sean (Dec 24 2019 at 07:33):

Sean (Dec 24 2019 at 07:34):

if you did kernel development, you might become familiar with the C offsetof() macro which this is modeled after but portably cross-platform

Sean (Dec 24 2019 at 07:34):

Jeffrey Liu (Dec 24 2019 at 07:34):

Ah ok, I think I'm starting to understand it more now. Thanks so much for your help! Hope you feel better soon :)

Jeffrey Liu (Dec 25 2019 at 09:37):

Hey @Sean , I mentioned a couple of extra changes I did on to the patch after my submission was approved - it seems like there were some things that I initially neglected to change. With the changes, the tests I've done seemed to have worked correctly (with the file input being default for the time being). Sorry for the inconvenience!

Jeffrey Liu (Dec 25 2019 at 09:39):

Here's the updated patch by the way - most of it was just me forgetting to change eip->file to eip->name after changing rt_ebm_internal struct parameter.
ebm2_updated.patch

Jeffrey Liu (Dec 25 2019 at 09:44):

Also one last minor thing, while I was looking through typein.c, I noticed that the first line for p_dsp_v5, which is "Take data from file or database binary object [f|o]:" is missing a space at the end while the others all have one. Even though it's not related to EBM, could I add on the space for my next patch?

Jeffrey Liu (Dec 25 2019 at 18:14):

Oh by the way, since C doesn't support function overloading, I'm getting an error when I try to add get_file_data() into src/librt/primitives/ebm/ebm.c because it already exists in src/librt/primitives/dsp/dsp.c; any idea how I could work around this?

Jeffrey Liu (Dec 25 2019 at 18:18):

It seems like the easiest option would be to just rename the functions in both files to something more specific like get_ebm_file_data() and get_dsp_file_data(), but I didn't want to change anything externally without permission. Would this solution be ok?

Thusal Ranawaka (Dec 26 2019 at 01:02):

starseeker (Dec 26 2019 at 01:37):

@Jeffrey Liu Hm. What about marking the functions as static? Do they need to be externally visible beyond their own files?

Jeffrey Liu (Dec 26 2019 at 01:40):

Oh, that works as well since they're only needed within the file. Thanks for the advice!

Jeffrey Liu (Dec 26 2019 at 01:50):

Hmm... Looking at the HIDDEN macro definition, it should already be static, but I was getting some errors before

Jeffrey Liu (Dec 26 2019 at 01:51):

starseeker (Dec 26 2019 at 03:08):

I would just use static directly, without using HIDDEN - I don't know that we'll be keeping HIDDEN in the long run anyway

starseeker (Dec 26 2019 at 03:09):

Hidden conditionally sets static, IIRC - Sean can give you more of the history there.

starseeker (Dec 26 2019 at 03:18):

Oh, right - it's in the common.h definition comment. Evidently someone found it useful to disable static for debugging purposes. I think there have been some discussions about the utility of that vs. losing the locality to the file that static is supposed to bring - my own thought is that it would be better to manually change the function names around in the (rare) case where we want to un-static a function for debugging and use static directly in its normal behavior all other times, but I don't remember for sure if that's where the consensus landed.

Jeffrey Liu (Dec 26 2019 at 03:49):

Ah ok, thanks for the explanation! I was just using it because it was being used in dsp.c - should I change it from HIDDEN to static in both dsp.c and ebm.c?

starseeker (Dec 26 2019 at 08:54):

@Jeffrey Liu You can, but shouldn't have to - as long as the ebm version is static, it shouldn't interfere with the dsp version.

Jeffrey Liu (Dec 27 2019 at 04:30):

Hey @Sean , for the fourth task, I noticed that there are other functions that currently rely upon the fact that EBM only supports file data at the moment. Should I update them as well?

Jeffrey Liu (Dec 27 2019 at 04:32):

For example, rt_ebm_prep or rt_ebm_bbox seem to rely on eip->mp, unless I'm mistaken?

Jeffrey Liu (Dec 27 2019 at 04:46):

I'm pretty sure get_obj_data() is working properly because a success statement is getting printed at the end of the function, but MGED still ends up crashing when I try to import from obj

Jeffrey Liu (Dec 27 2019 at 08:37):

I've been working for a while now but I still have no idea what's causing the crashing... this is my current patch: get_obj_data.patch , and the file reading is still working, but MGED is still crashing when I try to use a database object as the data source.

Jeffrey Liu (Dec 27 2019 at 08:37):

starseeker (Dec 27 2019 at 13:17):

starseeker (Dec 27 2019 at 13:18):

If you launch a debug build of MGED, attach the debugger and then try your ebm operation you should be able to see where it is wiping out...

Thusal Ranawaka (Dec 27 2019 at 13:19):

starseeker (Dec 27 2019 at 13:19):

Thusal Ranawaka (Dec 27 2019 at 13:20):

starseeker (Dec 27 2019 at 13:20):

@Thusal Ranawaka Remember to ask unrelated questions in another channel (this one is specific to the EBM primitive task)

starseeker (Dec 27 2019 at 13:24):

@Jeffrey Liu Without looking too closely at the code, I would expect that you'll need to update any of the other ebm functions that depend on a file being present for raytracing to work without crashing, but I'm not sure which functionality is called by the import command - this is a situation where the debugger would be helpful.

Jeffrey Liu (Dec 27 2019 at 17:14):

Ah ok, thanks for the advice! I didn't even know attaching debuggers was a thing - I'll try it out right now.

Jeffrey Liu (Dec 27 2019 at 17:44):

This was the exception indicated: pasted image I'll try to see what's going on but it definitely seems like it has to do with the file dependency

Jeffrey Liu (Dec 27 2019 at 17:48):

Jeffrey Liu (Dec 27 2019 at 17:52):

#define BIT(_eip, _xx, _yy) \
    ((unsigned char *)((_eip)->mp->apbuf))[ ((_yy)+BIT_YWIDEN)*((_eip)->xdim + BIT_XWIDEN*2)+(_xx)+BIT_XWIDEN ]

Jeffrey Liu (Dec 27 2019 at 17:53):

Jeffrey Liu (Dec 27 2019 at 17:56):

Or actually, it seems like this is the line throwing the actual exception base = y, but I think that's being caused by the BIT macro

Jeffrey Liu (Dec 27 2019 at 18:12):

Ok... so change (_eip)->mp->apbuf to (_eip)->buf solves the crashing, but now it looks like something is being read incorrectly which is probably a separate issue

Jeffrey Liu (Dec 27 2019 at 20:14):

I'm still having some issues with the BIT macro, object reading isn't crashing but it's producing some weird results, and now file reading is crashing with a similar issue as what was happening with the object reading before

Jeffrey Liu (Dec 27 2019 at 20:15):

I tried changing the logic of get_file_data() to the one found in dsp, because it looked like it was writing to eip->mp->apbuf before, but the issue is still there

Jeffrey Liu (Dec 28 2019 at 00:44):

I've tried everything I could think of so far, but nothing has been working for me. Depending on how I change BIT or get_file_data(), either file or object reading gets messed up. I'm pretty sure my change to BIT was correct, since other functions will rely on it to access the data, but even after changing get_file_data() to mimic the one from dsp.c, I'm still getting a read access violation during rt_ebm_plot().

Jeffrey Liu (Dec 28 2019 at 00:49):

What I don't understand is why it works for DSP, but not for this. From what I can tell, they're almost identical in terms of logic

Jeffrey Liu (Dec 28 2019 at 05:18):

For the weird results from object reading: removing the bit offsets from the BIT macro seems to fix it? File reading still isn't working though.

Jeffrey Liu (Dec 28 2019 at 05:19):

@Sean if you're available, could you help me with this? I still can't manage to get both working together.

Jeffrey Liu (Dec 28 2019 at 06:36):

My best guess is that since mp goes out of scope, the pointer gets freed while eip->mp is still pointing to that location, but if that's the case, then I'm not sure why it works for dsp?

Jeffrey Liu (Dec 28 2019 at 07:22):

Nope, I don't think that was it because I tried doing it without mp and the issue still occurs. This is what I'm currently trying file_reading_error.patch and this is the error that is occurring when I attach a debugger to MGED:

Exception thrown: read access violation.
eip->**buf** was 0x1110347A42F0112. occurred

starseeker (Dec 28 2019 at 17:33):

@Jeffrey Liu All I can suggest without stepping it through a debugger (unfortunately I don't have time for right now) is to look very carefully at each piece of the macro in question and what is in memory at the specific memory addresses in question, to make sure you understand what it intends to do vs what it is actually doing. Another action sometimes helpful is to expand the macro to the actual code being compiled and see if that is easier to follow.

Jeffrey Liu (Dec 28 2019 at 17:55):

Thanks for the advice! I'll look through it again and hopefully I can make some progress now that I've taken a break from it for a while

Jeffrey Liu (Dec 28 2019 at 20:03):

Hm.. so I'm trying to track the differences in execution between DSP and EBM, and I think the main difference is that this conditional gets evaluated differently (bu_cv_optimize(in_cookie) != bu_cv_optimize(out_cookie)), where DSP evaluates this as true whereas EBM evaluates it as false. I'll have to look more into the function itself, but the values of in_cookie and out_cookie are different?

Jeffrey Liu (Dec 28 2019 at 20:07):

Oh, so I guess the output of bu_cv_optimize end up becoming the same for in_cookie and out_cookie even though they're initially different.

Sean (Dec 28 2019 at 20:08):

Jeffrey Liu (Dec 28 2019 at 20:08):

Sean (Dec 28 2019 at 20:08):

I have a lot of messages to catch up on, but one thing to notice is that the datatype for EBM and DSP are different

Sean (Dec 28 2019 at 20:09):

Jeffrey Liu (Dec 28 2019 at 20:09):

I replaced "s" with "c" and 16 with 8 anywhere I could notice, but perhaps I might have missed something

Sean (Dec 28 2019 at 20:10):

the original ebm code would have been correct, it's just the translation needed for reading it from an in-memory correctly that should be needed, and to make sure it's not getting double-converter

Sean (Dec 28 2019 at 20:11):

Jeffrey Liu (Dec 28 2019 at 20:11):

Sean (Dec 28 2019 at 20:12):

Jeffrey Liu (Dec 28 2019 at 20:14):

I think I tried something like eip->buf = (unsigned char *)eip->mp->apbuf at the end of the original code before, but it still didn't work. I'll try it again just to make sure though

Jeffrey Liu (Dec 28 2019 at 20:16):

Sean (Dec 28 2019 at 20:17):

Sean (Dec 28 2019 at 20:18):

Sean (Dec 28 2019 at 20:19):

previously, for file-based source, ebm mapped the file into memory with bu_open_mapped_file_with_path()

Sean (Dec 28 2019 at 20:19):

then it "translates" it into ebm in-memory form with a simple memcpy() loop over the data

Sean (Dec 28 2019 at 20:21):

for dsp, the translation is a bit more complicated because it's 2-byte data and it has to make sure the bytes are in the right order (ABABAB... vs BABABA...)

Sean (Dec 28 2019 at 20:21):

Jeffrey Liu (Dec 28 2019 at 20:22):

Ohh, that makes sense, so I guess that means my current object reading is also incorrect then?

Sean (Dec 28 2019 at 20:22):

rather, it can be just a copy. heck, it probably doesn't not even need to copy it for the binunif case since its already in memory

Sean (Dec 28 2019 at 20:23):

Jeffrey Liu (Dec 28 2019 at 20:23):

It was pretty much the same thing as the dsp version, with changes to indicate that it was unsigned char

Jeffrey Liu (Dec 28 2019 at 20:24):

static int
get_obj_data(struct rt_ebm_internal *eip, const struct db_i *dbip)
{
    struct rt_binunif_internal *bip;
    int in_cookie, out_cookie;
    size_t got;
    int ret;

    BU_ALLOC(eip->bip, struct rt_db_internal);

    ret = rt_retrieve_binunif(eip->bip, dbip, eip->name);
    if (ret)
    return -1;

    if (RT_G_DEBUG & RT_DEBUG_HF) {
    bu_log("db_internal magic: 0x%08x  major: %d  minor: %d\n",
        eip->bip->idb_magic,
        eip->bip->idb_major_type,
        eip->bip->idb_minor_type);
    }

    bip = (struct rt_binunif_internal *)eip->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)(eip->xdim*eip->ydim))
    {
    size_t i = 0;
    size_t size;
    struct bu_vls binudesc = BU_VLS_INIT_ZERO;
    rt_binunif_describe(&binudesc, eip->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",
           eip->name,
           DB5_MINORTYPE_BINU_8BITINT_U,
           bip->type,
           (size_t)(eip->xdim * eip->ydim),
           bu_vls_cstr(&binudesc));
    return -2;
    }

    in_cookie = bu_cv_cookie("nuc"); /* data is network unsigned char */
    out_cookie = bu_cv_cookie("huc");

    if (bu_cv_optimize(in_cookie) != bu_cv_optimize(out_cookie)) {
    /* if we're on a little-endian machine we convert the input
     * file from network to host format
     */

    got = bu_cv_w_cookie(bip->u.uint8, out_cookie,
                 bip->count * sizeof(unsigned char),
                 bip->u.uint8, in_cookie, bip->count);

    if (got != bip->count) {
        bu_log("got %zu != count %zu", got, bip->count);
        bu_bomb("\n");
    }
    }

    eip->buf = bip->u.uint8;
    return 0;
}

Sean (Dec 28 2019 at 20:24):

so this is to get it from a binunif ... which assumes you created the binunif correctly

Jeffrey Liu (Dec 28 2019 at 20:26):

Wait, was this not how it was supposed to look? My understanding was that the process would be something like bo -i u C dbobj bw-image.bw and then in ebm ebm o dbobj 256 256 20, which is how I've been creating DSPs from database objects.

Sean (Dec 28 2019 at 20:28):

no, I think that's right ... and I wasn't suggesting it wasn't right, just that the code obviously needs it to BE right, not just assume it's right :)

Jeffrey Liu (Dec 28 2019 at 20:29):

Whew, I was scared that I was approaching this completely incorrectly... the issue I had with using the original EBM code for file reading and this code for the object reading was that the BIT macro got messed up

Jeffrey Liu (Dec 28 2019 at 20:31):

Sean (Dec 28 2019 at 20:32):

you're definitely on the right track. if anything, you're just a bit too complicated in how closely your mimicing the dsp code because it's got a more complicated data format

Sean (Dec 28 2019 at 20:33):

but it's not to say it can't work. it definitely can. just need to check data at every step along the way.

Sean (Dec 28 2019 at 20:34):

Jeffrey Liu (Dec 28 2019 at 20:37):

#define BIT(_eip, _xx, _yy) \
    ((unsigned char *)((_eip)->mp->apbuf))[ ((_yy)+BIT_YWIDEN)*((_eip)->xdim + BIT_XWIDEN*2)+(_xx)+BIT_XWIDEN ]

Jeffrey Liu (Dec 28 2019 at 20:37):

Jeffrey Liu (Dec 28 2019 at 20:41):

Oh so for the object reading, is the whole part with the bu_cv... unnecessary, since there's no need for complicated translations?

Sean (Dec 28 2019 at 20:41):

Sean (Dec 28 2019 at 20:42):

it's using 1-BIT data but since data is only stored on a byte basis, we must index into each byte to find the specific bit we're looking for

Sean (Dec 28 2019 at 20:42):

Sean (Dec 28 2019 at 20:44):

so looking at your get_obj_data() function, the only issue I see is all the bu_cv logic should be completely unnecessary. should take it out.

Sean (Dec 28 2019 at 20:45):

bip->u.uint8 is the data, so setting it to eip->buf should be adequate I believe

Jeffrey Liu (Dec 28 2019 at 20:46):

Ah ok, thanks for the explanation! I was pretty confused about what that part was for

Jeffrey Liu (Dec 28 2019 at 20:49):

So about the macro again, the issue I was having was that for file-based sources, the data would be stored in eip->mp->apbuf whereas the data would be stored in eip->buf for the object-based sources, so I would be dereferencing a null pointer

Jeffrey Liu (Dec 28 2019 at 20:52):

But if I changed the BIT macro to (_eip)->buf, the file reading logic would get messed up instead

Jeffrey Liu (Dec 28 2019 at 21:38):

One potential solution: switch-casing eip->datasrc anywhere BIT is called, and then passing eip->buf or eip->mp->apbuf depending on whether it is file-based or object-based?

Sean (Dec 28 2019 at 21:40):

yep, that or better still, replace BIT with a function that has the switch or other conditional to access the appropriate container

Jeffrey Liu (Dec 28 2019 at 21:41):

Ah, so I could do something like test if eip->buf is null or not, and if it is, use eip->mp->apbuf and if it isn't, use it?

Sean (Dec 28 2019 at 21:41):

Jeffrey Liu (Dec 28 2019 at 21:41):

Jeffrey Liu (Dec 28 2019 at 21:42):

Sean (Dec 28 2019 at 21:42):

for what it's worth, this is very much "real" coding that one might do on a day-to-day basis ;)

Jeffrey Liu (Dec 28 2019 at 21:45):

This is quite an experience, I think I'm slowly getting used to it :) this is the first time I've done "serious" debugging before

Jeffrey Liu (Dec 29 2019 at 07:01):

I feel like I'm so close but I can't get past this one error... I changed the macro to

#define BIT(_eip, _xx, _yy) (                                               \
    ((_eip)->datasrc == RT_EBM_SRC_FILE) ?                                      \
    ((unsigned char *)((_eip)->mp->apbuf))[ ((_yy)+BIT_YWIDEN)*((_eip)->xdim + BIT_XWIDEN*2)+(_xx)+BIT_XWIDEN ] \
    : ((unsigned char *)((_eip)->buf))[ (_yy)*((_eip)->xdim)+(_xx)])

but the error I'm getting is "expression must be an lvalue or function designator." I've read that as long as both the second and third arguments to the ternary have the same lvalue/type, then the type of the ternary expression is also an lvalue. Both expressions don't create any errors individually, so why is this error still occurring?

Sean (Dec 29 2019 at 07:05):

Sean (Dec 29 2019 at 07:06):

that is still crazy hard to read -- that's why I suggested turning it into a function anyways. macros are not only hard to read. they can be hard to debug too.

Jeffrey Liu (Dec 29 2019 at 07:06):

Jeffrey Liu (Dec 29 2019 at 07:07):

Jeffrey Liu (Dec 29 2019 at 07:09):

Jeffrey Liu (Dec 29 2019 at 07:19):

unsigned char *
BIT(struct rt_ebm_internal *eip, size_t x, size_t y)
{
    unsigned char *ret;

    switch (eip->datasrc) {
    case RT_EBM_SRC_FILE:
        ret = (((unsigned char *)(eip->mp->apbuf))[(y+BIT_YWIDEN)*(eip->xdim+BIT_XWIDEN*2)+x+BIT_XWIDEN]);
        break;
    case RT_EBM_SRC_OBJ:
        ret = ((eip->buf)[(y)*(eip->xdim) + x]);
        break;
    }
    return ret;
}

Sean (Dec 29 2019 at 07:21):

Sean (Dec 29 2019 at 07:22):

Sean (Dec 29 2019 at 07:23):

it's a temporary value, might be in a register, might be handled by the compiler in some obscure way, might not even exist if inlined or eliminated (though this case it wouldn't be)

Sean (Dec 29 2019 at 07:23):

Sean (Dec 29 2019 at 07:25):

Jeffrey Liu (Dec 29 2019 at 07:29):

I think that fix worked, thanks so much! I'm still a little confused though, could you elaborate on the difference between the address of the function return value and the actual address?

Sean (Dec 29 2019 at 07:34):

Sean (Dec 29 2019 at 07:35):

it evaluates to the address of something in memory, which canonically is the address of a variable

Sean (Dec 29 2019 at 07:35):

Sean (Dec 29 2019 at 07:36):

Jeffrey Liu (Dec 29 2019 at 07:36):

Sean (Dec 29 2019 at 07:36):

a function returns a value, but you can't take the address of a value .. you take the address of something in memory

Sean (Dec 29 2019 at 07:36):

Sean (Dec 29 2019 at 07:37):

Jeffrey Liu (Dec 29 2019 at 07:38):

Sean (Dec 29 2019 at 07:38):

you won't really fully understand probably until you attempt to do a canonical bit of recursive programming

Sean (Dec 29 2019 at 07:39):

a good way to think about it, though, is to think about memory and where things are in memory

Sean (Dec 29 2019 at 07:40):

a value being returned from a function doesn't necessarily reside in memory, thus you simply can't get the address to it until you assign it to a variable (i.e., until you save that value into a piece of memory)

Jeffrey Liu (Dec 29 2019 at 07:41):

That makes sense I think, I guess I never knew that that was how function return values worked

Sean (Dec 29 2019 at 07:41):

Sean (Dec 29 2019 at 07:42):

it's a little different for C as it doesn't have the concept of references, but the rest applies

Jeffrey Liu (Dec 29 2019 at 07:48):

Hm, for some reason the function (or more specifically, ret = (((unsigned char *)(eip->mp->apbuf))[(y+BIT_YWIDEN)*(eip->xdim+BIT_XWIDEN*2)+x+BIT_XWIDEN]);) keeps returning NULL

Sean (Dec 29 2019 at 07:50):

well you had a couple mistakes, for example you had it returning a pointer yet the indexing is into an unsigned char array

Sean (Dec 29 2019 at 07:51):

but then that'd be a problem for memcpy as you need the address of that unsigned char

Sean (Dec 29 2019 at 07:53):

what may work is to move the address-of operator into the function, but then you'll need to modify all of the calls

Jeffrey Liu (Dec 29 2019 at 07:56):

Oh, so to get the address of the unsigned char, I would just place the & operator in front of that whole expression, right?

Jeffrey Liu (Dec 29 2019 at 07:57):

And then in places where the value itself is needed, I would need to dereference it?

Jeffrey Liu (Dec 29 2019 at 08:04):

Also for that case, wouldn't something like memcpy(BIT(eip, 0, y), cp, eip->xdim); be enough, since the value is now the address?

Jeffrey Liu (Dec 29 2019 at 08:06):

Oops sorry I didn't realize I just asked 3 questions in a row like that, I'm pretty inexperienced working with pointers and all of their intricacies

Jeffrey Liu (Dec 29 2019 at 08:16):

Well as an update - for the most part, it actually worked!! The only thing that's buggy are the outlines around the image during object reading: pasted image

Jeffrey Liu (Dec 29 2019 at 08:47):

My best guess is that it has something to do with the fact that the file-based bitmap is stored with 2 cells of 0s around, but something changes for the object-based bitmap, so it's accessing random external values.

Jeffrey Liu (Dec 29 2019 at 09:15):

Yeah, it definitely looks like the data is being read at some weird offset or something - I'll see if I can figure out what's causing it tomorrow. Thanks so much for all of your help @Sean and @starseeker ! I think I learned a lot from this whole process.

Jeffrey Liu (Dec 29 2019 at 22:59):

Well, I think I managed to work around it by copying the data from bip->u.uint8 along with the specified offset, but if there's a solution that doesn't require copying, that would probably be better.

Jeffrey Liu (Dec 31 2019 at 19:05):

Hi @starseeker , I noticed that r74585 reverted an EBM change I made because it broke the regress-solids regression test. While that patch was still incomplete (I just submitted my new patch that can implement both file and object source), is there something that I should correct?

Jeffrey Liu (Dec 31 2019 at 19:12):

starseeker (Jan 01 2020 at 02:48):

starseeker (Jan 01 2020 at 02:52):

Er, hmm... now that I think about it, I don't know that that test will run on Windows...

starseeker (Jan 01 2020 at 02:53):

Jeffrey Liu (Jan 01 2020 at 04:44):

Sorry, I didn't look much into it, I just noticed what the revision note that reverted the EBM changes said (although I guess I wouldn't be able to anyways, since I'm on Windows?). I was only wondering if there was anything I would be able to change to EBM in order to somehow correct this issue.

Sean (Jan 01 2020 at 07:44):

Sean (Jan 01 2020 at 07:50):

@starseeker I'll have to reproduce/see the regression failure. A little surprised it failed anything as it was a non-logic change. Gut reaction would be to assume it's a bad test / false-positive but maybe something going on with structparse printing. There's a bigger patch pending that will get more exhaustively tested sitting as an sf patch waiting for it all to come together.

Jeffrey Liu (Jan 01 2020 at 15:58):

I didn't look at every single function in EBM, but fortunately, I think most of them were originally using the BIT macro to access the data. Hopefully, now that I've replaced all the BIT calls with the new bit() function, it should mostly be correct already :)

Sean (Jan 02 2020 at 02:49):

It's definitely what I would expect as well. It should be right or at least really close to correct.

Sean (Jan 02 2020 at 07:20):

@Jeffrey Liu So I did a quick test of creating an ebm using an older/existing MGED, then opened that ebm using an MGED with your patch applied, and it fails to load it correctly:

Sean (Jan 02 2020 at 07:20):

mged> tops
00000.000000.comb/   ebm

mged> e ebm
Unreasonable EBM parameters
    file="data"
    src=''
    w=32

    n=32

    d=10

 mat=1.000000E+00 0.000000E+00 0.000000E+00 0.000000E+00

0.000000E+00 1.000000E+00 0.000000E+00 0.000000E+00

0.000000E+00 0.000000E+00 1.000000E+00 0.000000E+00

0.000000E+00 0.000000E+00 0.000000E+00 1.000000E+00
rt_db_external5_to_internal5(ebm):  import failure
db_recurse() rt_db_get_internal(ebm) FAIL
mged> l ebm
Unreasonable EBM parameters
    file="data"
    src=''
    w=32

    n=32

    d=10

 mat=1.000000E+00 0.000000E+00 0.000000E+00 0.000000E+00

0.000000E+00 1.000000E+00 0.000000E+00 0.000000E+00

0.000000E+00 0.000000E+00 1.000000E+00 0.000000E+00

0.000000E+00 0.000000E+00 0.000000E+00 1.000000E+00
rt_db_external5_to_internal5(ebm):  import failure
rt_db_get_internal(ebm) failure

mged> in ebm2
Enter solid type: ebm
Take data from file or database binary object [f|o]: f
Enter name of file/object: data
Enter width of bit-map (number of cells): 32
Enter height of bit-map (number of cells): 32
Enter extrusion distance: 10
ebm2
mged> l ebm2
ebm2:  extruded bitmap (EBM)
      file="data" w=32 n=32 depth=10
   mat=1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1

Sean (Jan 02 2020 at 07:23):

As you can hopefully see, it works just fine creating new ebm, but can't handle old ebm (and it obviously should). I think the issue is the empty src='' field, but hopefully something you can recreate and fix. It could be something as simple as setting a default value in the structparse table, or it might require some change in the ebm import5 function. It's unclear why import5 is reporting a failure, but that's where the problem resides.

Sean (Jan 02 2020 at 07:24):

This is likely why the original patch had to be reverted as well as we have tests that include old ebm.

Jeffrey Liu (Jan 02 2020 at 09:48):

Ah I forgot to account for that... so if I'm understanding correctly, the issue lies in the fact that EBM objects created before the datasrc field was added, so then redrawing them after the update will cause an error because nothing was written to that field for those objects?

Jeffrey Liu (Jan 02 2020 at 10:08):

I think one method that would work would be to add a check like !eip->datasrc and then assign it a value of RT_EBM_SRC_FILE. I tried something like this, {"%c", 1, "src", RT_EBM_O(datasrc), BU_STRUCTPARSE_FUNC_NULL, NULL, 'f' } but I guess I either did something wrong or it didn't work

Jeffrey Liu (Jan 02 2020 at 10:11):

Another option could be to add a function like hook_verify() found in DSP to do the checking, if that's how those functions work?

Jeffrey Liu (Jan 02 2020 at 10:19):

Ok, so I'm pretty sure this check worked: if (!eip->datasrc) eip->datasrc = RT_EBM_SRC_FILE;
I currently just placed it right before import5() checks for reasonable values, but I could also try putting it in something like a hook_verify() if that's preferred.

Jeffrey Liu (Jan 02 2020 at 10:26):

Here's the patch: ebm4_updated6.patch
I think it should work for all cases because MGED inherently prevents the user from inputting nothing to a field.

Jeffrey Liu (Jan 02 2020 at 10:31):

Drawing/erasing "old" EBM objects works now, but could there be some other functions that I haven't accounted for through this check?

Sean (Jan 02 2020 at 15:46):

simple is fine, no need for a hook function if putting a check in one place fixes it. dsp needed the check in a few places (v4 and v5 support), so it then makes sense to refactor to a function that does the check.

Sean (Jan 02 2020 at 15:47):

that way if the check itself ever needs to change, you change it in one place instead of finding everywhere datasrc is accessed and having to re-evaluate if it's needed

Sean (Jan 02 2020 at 15:48):

This I don't know, wouldn't know without reading the code in more detail. You could look at all the places ->name is accessed and see if any seem to assume it's a file.

Jeffrey Liu (Jan 02 2020 at 21:45):

I just looked at all the calls to ->name, as well as if there were any more calls to ->mp->apbuf (how the data was accessed before), but it seemed okay to me. It looks like most functions were using BIT (now bit()) which checks the datasrc.

Jeffrey Liu (Jan 02 2020 at 21:49):

For the wiki edit task, could I include everything that should be added into a pdf file? Some of the old text would also need to be adjusted as well, so should I include that in the comments?

Jeffrey Liu (Jan 03 2020 at 20:50):

Hi, I was trying to test the solid edit mode feature for DSP and EBM, but I was having a little trouble working with it and I couldn't find much relevant documentation. My main issue was that when trying to edit something, my input is only ever prompted when I choose file name. Otherwise, I'm not really sure how I'm supposed to edit the value (such as file size). Could anyone help me with this?

Jeffrey Liu (Jan 03 2020 at 20:54):

Also, I think I may have found a bug (I don't think it's been reported before, so could I submit this as a bug report task?) - when I try to use "Primitive Editor" and I try to select a DSP object that I have created, I get this error after I click "Reset:" pasted image

Sean (Jan 03 2020 at 21:42):

There's not really any documentation on the Tcl side of things besides the code itself.

Sean (Jan 03 2020 at 22:06):

As to the file name prompt, that's because the code makes a call that asks for the file dialog.

Sean (Jan 03 2020 at 22:11):

That's really the central issue with the task, figuring out how to conditionally prompt differently. I would suggest changing the menu entry to a "Data Source" entry that opens a dialog that has a "Object or file name" label, a text input where you can type a name, and a button next to it that opens the file dialog. Third line could have a toggle input that denotes object or file that autosets to file when the file dialog is used but otherwise defaults to object. Could display warning when the name doesn't match an existing object or file accordingly.

Sean (Jan 03 2020 at 22:12):

This is kind of what happens when you edit a sketch object, it opens a new window.

Sean (Jan 03 2020 at 22:17):

The easiest way to go about this is probably to just do a simple tk tutorial that creates a little window with the input field, file button, checkbox, labels, etc. Get that working, then you just have to make the menu call it.

Sean (Jan 03 2020 at 22:18):

Sean (Jan 03 2020 at 22:27):

A good example to follow might be the color selection panel which you can see under Tools -> Color Selector or by searching for "cadColorWidget" in src/tclscripts/mged (most is in colors.tcl)

Sean (Jan 03 2020 at 22:27):

Jeffrey Liu (Jan 04 2020 at 00:57):

Ah okay, thank you for the advice! I think I have a better idea of what the task is asking for now - would it be better to reprompt for the file size as well when a new file/object is chosen?

Jeffrey Liu (Jan 04 2020 at 02:20):

On second thought, it might be better to leave them separate. I'm still a little confused about how to precisely edit values though, when I'm in primitive edit mode for EBM and try to do Edit > File Size (W N), I'm not given a prompt but middle clicking doesn't do anything either. Looking at edsol.c, it looks like it's extracting 2 arguments from es_para which is keyboard input, but I still haven't been able to actually input new dimensions myself. Am I still doing something wrong?

Jeffrey Liu (Jan 04 2020 at 07:12):

@Sean I just made this on Google Drawings to plan things out, but is this similar to what you were talking about? pasted image

Jeffrey Liu (Jan 04 2020 at 07:15):

I haven't used Tcl/Tk before, but I took a look at the existing code and I think two challenges might be returning two different parameters (name and datasrc), and actually calling the Tcl getFile command to get a file name.

Jeffrey Liu (Jan 04 2020 at 21:34):

Well, I think an easy way to return both name and datasrc would be to just concatenate the two before returning it before parsing it to separate them. I'm having some difficulty actually creating the window though - I took a look at color.tcl and botedit.tcl, but from what I understand, those are directly called through openw.tcl (and some variables are provided) whereas getFile() is called through edsol.c. Is there something that I overlooked?

Jeffrey Liu (Jan 04 2020 at 21:39):

I've been trying a couple soledit functions, but so far I haven't found any that actually opens a new window aside from the call to tk_getOpenFile

Wendelin Wemhöner (Jan 04 2020 at 21:47):

Hey! My name is Wendelin and I'm participating in the Google Code In. My favorite task by BRL-CAD is the "Discover the BRL-CAD open source community" task because it is a lovely introduction to this project.

Sean (Jan 05 2020 at 07:37):

@Jeffrey Liu I don't think it needs to prompt for file size as that's already a menu option unless you want to completely implement an edit panel for all the ebm parameters (kind of like how 'sketch' object editing is handled)

Sean (Jan 05 2020 at 07:39):

it's not that it'd better or worse. both / either can work. that said, I think you found a bug in the File Size (W N) edit menu. The normal way to set values on the menu (like the extrusion height) is to either middle-click in the gui or utilize the 'p' command to provide exact values. So you'd select "File Size ..." and then run something like "p 10 20".

Sean (Jan 05 2020 at 07:40):

However, like I said, it doesn't appear to be working right now. Some bug in src/mged/edsol.c before where it reports "width and length of file are required".

Sean (Jan 05 2020 at 07:44):

Yes, very close. Welcome to ideas, but could also just have two buttons -- select data object & select data file. then you wouldn't need the toggle since it'd be implicit in the selection.

Sean (Jan 05 2020 at 07:56):

Sean (Jan 05 2020 at 07:57):

Jeffrey Liu (Jan 05 2020 at 09:12):

Thank you for the advice! The 'p' command was exactly what I was looking for. Regarding the references - I took a look at them, but from what I could understand, they were being called directly from openw.tcl, which passed in a couple variables ($id, $screen) that were used to actually create the window. On the other hand, the Tk application that I would need to make would be called from edsol.c, right? That's why I was a little confused.

Jeffrey Liu (Jan 05 2020 at 09:51):

I just tested this, and I would first get the error "three arguments needed," but when I put in 3 arguments, I would get the error "width and length of file are required." I looked into it and this seems to be the problem:

else {
    if (inpara != 3) {
        Tcl_AppendResult(interp, "ERROR: three arguments needed\n", (char *)NULL);
        inpara = 0;
        return TCL_ERROR;
    }
}

on line 7902, as everything worked normally after I commented it out. Perhaps the issue is that the if statement is not being evaluated correctly?

Jeffrey Liu (Jan 05 2020 at 09:55):

Or actually in this case, there might need to be an else-if statement because the if block checks for editing options with only a single argument.

Jeffrey Liu (Jan 05 2020 at 10:12):

The if conditional is a little interesting though, I'm getting similar "three arguments needed" errors when I try to set EBM height or scale DSP X/Y, all of which work again after I comment out the else block

Jeffrey Liu (Jan 05 2020 at 10:19):

Back to my challenge with creating a new window, this was what I was talking about:
proc bot_askforname {parent screen}
where the procedure is already given certain arguments beforehand, which it uses to create a new window. However, I believe that when calling a function through edsol.c, I don't have this capability.

Jeffrey Liu (Jan 05 2020 at 19:52):

At the risk of spamming too many things, I think I found another error: using tk_getOpenFile returns the absolute path, which gets assigned to ebm->name (something similar happens for DSP). However, DSP and EBM both use the function bu_open_mapped_file_with_path(), which assumes a relative path. As a result, it concatenates the paths weirdly, and I'm getting a file import error whenever I try to edit a DSP or EBM file name/path. Perhaps it might be better to just prompt the user to manually type a name and then choose file or object, as this is essentially what DSP/EBM creation does in the first place?

Jeffrey Liu (Jan 07 2020 at 00:46):

Ahh I finally got to create a new window - I needed to call dm_get_filepath(DMP) and dm_get_dname(DMP) in order to pass parent and screen as arguments to the function. As for the two errors - I mentioned possible solutions above, but perhaps if a dev could confirm some things so that I don't go ahead to change some things that shouldn't be changed?

starseeker (Jan 07 2020 at 02:11):

What about using bu_dir to get the root of the filepath, then passing the relative path on to the creation routine?

Jeffrey Liu (Jan 07 2020 at 05:07):

Shoot, I just saw this but I'm almost done with my implementation that requires a manual input. I think I'll stick with this for now since it's already taken me quite a while to get to an almost complete GUI. I'm just having trouble returning a string using the information from the entry and menubutton widget.

Jeffrey Liu (Jan 07 2020 at 06:40):

I'm so close to finishing the GUI, but I'm stuck on one last thing. Could I get some help? This is my code (I know it's pretty messy right now, but I'll clean it up once I get things working):

proc apply_datasrc { type name } {
    if {$type == "File"} {
    set ret "f"
    } else {
    set ret "o"
    }
    append ret $name
    return $ret
}

proc init_datasrc { parent screen } {
    set w $parent.data
    catch { destroy $w }

    set row -1
    toplevel $w -screen $screen
    wm title $w "Set Data Source"

    incr row
    frame $w.nameF -relief flat -bd 2
    label $w.nameL -text "Name:" -anchor e
    entry $w.nameE -relief sunken -bd 2 -textvariable name
    grid $w.nameL $w.nameE -in $w.nameF -padx 8
    grid $w.nameF -row $row -column 0 -sticky nsew -pady 8
    grid rowconfigure $w $row -weight 0

    incr row
    frame $w.typeF -relief flat -bd 2
    label $w.typeL -text "Type:" -anchor e
    menubutton $w.typeMB -relief raised -bd 2 -text "Object" -textvariable type -menu $w.typeMB.m
    menu $w.typeMB.m -tearoff 0
    $w.typeMB.m add command -label Object -command {set type "Object"}
    $w.typeMB.m add command -label File -command {set type "File"}
    grid $w.typeL $w.typeMB -in $w.typeF -padx 8
    grid $w.typeF -row $row -column 0 -sticky nsew -pady 8
    grid rowconfigure $w $row -weight 0

    incr row
    frame $w.buttons -relief flat -bd 2
    button $w.okB -text "OK" \
    -command {set ret [apply_datasrc $type $name]; return $ret}
    button $w.dismissB -text "Dismiss" \
    -command "destroy $w"
    grid $w.okB x $w.dismissB -in $w.buttons -sticky nsew -padx 8
    grid columnconfigure $w.buttons 1 -weight 1
    grid $w.buttons -row $row -column 0 -sticky nsew -pady 8
    grid rowconfigure $w $row -weight 0

    grid columnconfigure $w 0 -weight 1
}

proc getDataSrc { parent screen } {
    set ret [init_datasrc $parent $screen]
    return $ret
}

So the issue I'm having is that when getDataSrc is called, the GUI gets set up and all, but even though I've tested that ret is getting assigned exactly what I want it to after I click "OK," the GUI remains open and it doesn't seem like anything gets updated. I've tried debugging but I'm still a little bit confused as to how the Tcl interpreter is working, so I was wondering if there could be something blatantly incorrect with this code?

Sean (Jan 07 2020 at 06:43):

@Jeffrey Liu manual is certainly fine for starters. I don't think the GUI editing method has been tested very much for dsp, so it's not surprising. full path should work, as should relative, so it's more likely a change to mapped file maybe broke that or requires a flag now.

Sean (Jan 07 2020 at 06:43):

we definitely do have much better file/path handling routines now than when dsp was implemented

Jeffrey Liu (Jan 07 2020 at 06:45):

Ah ok, thank you for the advice! If that's the case, perhaps I could try to update both DSP and EBM after GCi ends?

Sean (Jan 07 2020 at 06:47):

Jeffrey Liu (Jan 07 2020 at 06:48):

Haha, yeah I did notice that a couple of DSP things weren't updated/implemented yet (like the file size sedit option)

Sean (Jan 07 2020 at 06:49):

I don't see anything blatantly wrong in your code, but that doesn't mean the data is getting to where it needs to be. tcl debugging is annoying.

Sean (Jan 07 2020 at 06:50):

there is no return value from init_datasrc so returning that in getDataSrc presumably does nothing

Jeffrey Liu (Jan 07 2020 at 06:52):

Hm, I didn't know how I would explicitly declare a return value, but I thought that the "return" command from the OK button would suffice. I did create a label from within getDataSrc with ret as a textvariable, and it was getting updated too. That's why I'm a little confused as to why the string isn't getting returned back.

Sean (Jan 07 2020 at 07:08):

I don't think that's going to suffice because it's not a return from the right place

Sean (Jan 07 2020 at 07:31):

that said, don't worry about it too much. if you have it displaying a GUI, I'm happy to call the task done. that's useful in itself and more appropriately scoped. it was a noticeably harder task involving aspects of tcl/tk, mged, gui design, gui behavior implementation, debugging existing code, and hooking into existing code. task probably should have been a chain of 3-6 tasks ..

Jeffrey Liu (Jan 07 2020 at 13:01):

Alright, thanks so much! The tcl/tk was definitely challenging, but I wish I could've got everything working together though...

Jeffrey Liu (Jan 07 2020 at 13:05):

Erik (Jan 07 2020 at 20:31):

looking pretty awesome, @Jeffrey Liu , I don't suppose we could see a non-2^n non-square "complicated" bitmap (like a photo) raytraced? Eye candy is always nice. Keep up the good work!

Jeffrey Liu (Jan 07 2020 at 20:37):

Erik (Jan 07 2020 at 20:48):

Erik (Jan 07 2020 at 21:07):

(not a requirement or anything. Everyone likes eye candy and this could be a good acid test :slight_smile: )

Jeffrey Liu (Jan 07 2020 at 22:07):

Hm... It definitely sounds interesting, I never thought of using EBM (or did you mean DSP?) like that. It might seem a bit weird as an EBM, since everything would be the same height.

Jeffrey Liu (Jan 07 2020 at 22:07):

Jeffrey Liu (Jan 07 2020 at 22:17):

Shoot I just realized that I don't have BRLCAD compiled on Linux, I'll have to do that first

Sean (Jan 07 2020 at 22:30):

Sean (Jan 07 2020 at 22:45):

Jeffrey Liu (Jan 07 2020 at 23:12):

Jeffrey Liu (Jan 07 2020 at 23:13):

Although if the EBM reads the white pixels, would it be better to invert the colors?

Jeffrey Liu (Jan 08 2020 at 00:38):

I didn't really spend too much time configuring the colors and stuff, but here's a quick render of two images imported to EBM: lincoln_render.png tsunami_render.png

Sean (Jan 08 2020 at 00:40):

Jeffrey Liu (Jan 08 2020 at 00:43):

Jeffrey Liu (Jan 08 2020 at 00:52):

By the way @Sean , would it be alright for me to update the wiki before the patch is applied?

Sean (Jan 08 2020 at 00:52):

Jeffrey Liu (Jan 08 2020 at 00:53):

Ok thanks, I wasn't sure if I should just in case someone else would be referencing it anytime soon

Jeffrey Liu (Jan 08 2020 at 01:19):

Sean (Jan 08 2020 at 03:51):

@Jeffrey Liu Got it and closed. Thank you for your work on this. Hopefully you had fun.. that was a bit more of a diversion than was expected, but hopefully learned a lot too. ;)

Sean (Jan 08 2020 at 03:53):

The ebm work is actually going to be quite useful. What we really need is to generalize the entire concept of referencing something from a data object so that it's the same across dsp, ebm, vol, textures, uv maps, projection maps, image objects, and other entities instead of getting coded up independently each time. That's not GCI work, but it is where we need to get to long term.

Sean (Jan 08 2020 at 03:54):

Jeffrey Liu (Jan 08 2020 at 09:51):

Thank you :) Throughout the whole taskset, I learned a lot (Tcl/Tk, pointers, macros, etc) and I think I have a better understanding of how MGED works now. It took me a lot longer than I also expected, but the process as a whole was a lot of fun. As for the future work, would it be possible for me to contribute to this after GCI?

Jeffrey Liu (Jan 08 2020 at 09:53):

That said, I can't wait to get back into the appleseed tasks! But before that, I need to revert my EBM changes right?

starseeker (Jan 08 2020 at 12:22):

Sean (Jan 08 2020 at 16:47):

@Jeffrey Liu the HOPE is that you keep contributing after GCI. you can work on literally any aspect of the code that you desire. happy to mentor you in new or old areas. would be great to take on a specific project of your own that is interesting to you that you can keep working on.

Sean (Jan 08 2020 at 16:48):

No reason to revert them. You'll just want to be intentional when you "svn diff" anything in the future to make sure you don't pull in ebm changes.

Sean (Jan 08 2020 at 17:14):

@Jeffrey Liu can you submit the Great Wave model as a patch? that'd make a great sample ebm for the db/ directory to showcase the new embedded binunif ebm data source capability. You'll want to make sure the model is cleanly near the origin (with 0,0,0 at the bottom-left corner or base center), has a title set (e.g., Great Wave Off Kanagawa silhouette as an extruded bitmap by Jeffrey Liu), has good object names, has a region defined, etc., and also hooked into the CMakeLists.txt file so it gets installed. Think it'll need to be added as a .g as I don't believe binunif serialize to .asc yet.

Jeffrey Liu (Jan 08 2020 at 20:03):

No problem, I'll do that as soon as I can :) For future projects, I'd definitely like to first tie up any loose ends with EBM/DSP and appleseed (perhaps at a slightly more leisurely but thorough pace), and then find something else do to from there.

Jeffrey Liu (Jan 08 2020 at 20:09):

The image is pretty large though (it's around 3500x2500 if I recall), should I scale it down or find a smaller image so it takes up less space?

Jeffrey Liu (Jan 08 2020 at 20:50):

Yeah, the .g becomes almost 10mb with it, so I think it'd be better if I scaled it down a bit.

Sean (Jan 08 2020 at 21:34):

Sean (Jan 08 2020 at 21:35):

if it compresses well, you could have it be external, but then "compile" a version that has it imported as a binunif

Jeffrey Liu (Jan 08 2020 at 21:42):

Sorry, I'm not quite sure if I understand what you mean by compress. Does it have something to do with CMake?

Sean (Jan 08 2020 at 21:43):

Jeffrey Liu (Jan 08 2020 at 21:44):

Sean (Jan 08 2020 at 21:45):

Sean (Jan 08 2020 at 21:46):

if you have 7-zip installed, it has support for the bzip2 and gzip formats that do much better

Sean (Jan 08 2020 at 21:48):

cmake has built-in support for both too, so the general idea would be to add the image data as a compressed bz2 or gz file, then during "make", you'd run a custom command that decompresses the data file and imports it into a .g file.

Sean (Jan 08 2020 at 21:50):

if that's all too complicated, seriously, you can just add the .g file as-is. 10mb will probably compress down to less than 1mb but it's not a big deal. we have bigger .g's in there already.

Jeffrey Liu (Jan 08 2020 at 21:51):

Jeffrey Liu (Jan 08 2020 at 22:34):

Sean (Jan 08 2020 at 22:35):

Sean (Jan 08 2020 at 22:36):

something we've had on our roadmap for a long time is the ability to store objects compressed in the .g file, so storing a binunif compressed for example. that would be awesome for lots of things.

Jeffrey Liu (Jan 08 2020 at 22:45):

Sean (Jan 08 2020 at 22:46):

our file format specification has bits reserved to say "this is compressed" but no compression currently happens

Sean (Jan 08 2020 at 22:46):

probably wouldn't be too hard now that we have lz4 (a compression library) integrated

Jeffrey Liu (Jan 08 2020 at 23:12):

Oh wait, I just gzipped the geometry database with the image data already imported, is that okay? I noticed that the CMake text doesn't mention anything about .gz though, so would I need to include that somewhere as well?

Sean (Jan 08 2020 at 23:32):

@Jeffrey Liu Yeah, I think that's fine. You'll want to check out the cmake "tar" command. It has compression/decompression options. BZip2 ('j' option) is best.

Jeffrey Liu (Jan 09 2020 at 03:42):

Ah ok, thanks! So if I zipped it with BZip2, would it be enough to just include something like tar -E cjf tsunami.g.bz2 tsunami.g at the end of db/CMakeLists.txt?

Jeffrey Liu (Jan 09 2020 at 03:44):

Something seems a little off with that, but the ADD_G_TARGET function doesn't mention anything about zipped geometry database files so I'm not sure if I'm supposed to add it to G_SAMPLE_MODELS.

Sean (Jan 09 2020 at 08:40):

Sean (Jan 09 2020 at 08:43):

@Jeffrey Liu look at regress/icv/CMakeLists.txt for a better example, search for "tar "

Sean (Jan 09 2020 at 08:43):

Sean (Jan 09 2020 at 08:44):

Jeffrey Liu (Jan 09 2020 at 15:45):

Ok thanks, I'll check it out as soon as I can - I was having some trouble finding an example where tar was being used.

Jeffrey Liu (Jan 10 2020 at 02:07):

So for this, should I just leave the command outside of the ADD_G_TARGET function and exclude the .bz2 from the G_SAMPLE_MODELS? I think that if I left it in there, I would need to add a check for .bz2 files within the function itself. I'm perfectly fine with doing it, but I wasn't sure if that's making too many unwanted changes to the file.

Jeffrey Liu (Jan 10 2020 at 03:59):

^ so what I meant by that was I would do something like this: bz.patch I have no idea if this solution would work or not, but calling tar outside of the rest of the logic seemed a little bit messy

starseeker (Jan 10 2020 at 04:13):

Hmm. That might actually do fairly well - you may want to try an extension of .g.tbz2 to match what we're doing for the images - tar and bzip2 are generic to file type.

starseeker (Jan 10 2020 at 04:14):

I'd have to test if that logic can work with the more elaborate extension - it might not as-is.

starseeker (Jan 10 2020 at 04:20):

You've already got the key idea by adding the bz2 logic in the right place - the most likely failure point is needing to handle a two-part file extension with that REGEX expression - it may work as is, but I'd have to try it to be sure.

starseeker (Jan 10 2020 at 04:25):

@Jeffrey Liu You can actually use CMake itself to make the tbz2 file. On Linux it would be:

Jeffrey Liu (Jan 10 2020 at 11:34):

Thanks! The command worked for me as well. I haven't tested the actual logic itself yet, but I think I might want to set this aside for just a little bit so I can focus on my appleseed task.

starseeker (Jan 10 2020 at 13:04):

Jeffrey Liu (Jan 10 2020 at 18:09):

Actually, I did have one more question. What exactly is the DEPENDS macro used for? I noticed that all of the other add_custom_commands had it at the end, which also had the command as an argument. I was thinking about doing something like this:

if(${MODEL_TYPE} STREQUAL "g.tbz2")
    add_custom_command(
      OUTPUT ${CMAKE_CURRENT_BUILD_DIR_SCRIPT}/${output_file}
      COMMAND ${CMAKE_COMMAND} -E tar xjf ${CMAKE_CURRENT_SOURCE_DIR}/${in_model} WORKING_DIRECTORY ${CMAKE_CURRENT_BUILD_DIR}
      DEPENDS ${CMAKE_COMMAND} ${CMAKE_CURRENT_SOURCE_DIR}/${in_model}
    )
endif(${MODEL_TYPE} STREQUAL "g.tbz2")

starseeker (Jan 11 2020 at 02:23):

DEPENDS tells the build target that it is dependent on that file (or another build target). That way, if the input file or a target that is required by the current target changes, the build system knows it needs to recreate the .g file

Jeffrey Liu (Jan 11 2020 at 05:36):

Ah ok, thanks for the explanation! So I guess it would be better to include it then?

Jeffrey Liu (Jan 24 2020 at 00:13):

Before I forget, here's some of the preliminary stuff I did for the example ebm .g file: example_ebm.patch tsunami.g.tbz2 I'm almost positive it's building correctly, but in the wrong directory (build/db instead of build/Debug/share/db) - it's probably a pretty easy fix that I'll take care of later, I just haven't taken that close of a look at it for a while.

Sean (Jan 25 2020 at 05:46):

thanks @Jeffrey Liu ! Definitely don't want to loose track of this work. I was just testing the ebm code changes work recently. It's almost ready to integrate, but a few issues I had to track down.

Jeffrey Liu (Jan 25 2020 at 12:56):

Jeffrey Liu (Jan 25 2020 at 13:08):

Which also reminds me, there were a few minor bugs that I should submit tickets for relating to DSP/EBM

Sean (Jan 25 2020 at 21:30):

Jeffrey Liu (Jan 25 2020 at 21:41):

Yep, that would be the plan, but seeing as I'll probably be working mostly on Appleseed integration, I thought I'd submit tickets so that I don't lose track of the bugs I found (to be honest, I already had to scroll through the chat to recall what the issues were).

Sean (Jan 25 2020 at 21:41):

Jeffrey Liu (Jan 25 2020 at 21:46):

So I think I found 3/4 - one was the issue of tk_getOpenFile() returning an absolute path while bu_open_mapped_file_with_path() was expecting a relative path, another was the issue of editing the data source of a DSP object-based source (the file browser was still opening), another one was the issue of trying to select a DSP object with the primitive editor (this one is odd, since I'm getting 2 different errors on the version of MGED that I installed vs. the version of MGED compiled on a recent revision)

Jeffrey Liu (Jan 25 2020 at 21:48):

I've also had a weird issue with the 'p' command, where I keep getting "three arguments needed" when trying to edit attributes of EBM or DSP (I haven't tried other object types yet), but I'm surprised that nothing similar has been reported before from what I've seen.

Sean (Jan 25 2020 at 21:50):

"bu_open_mapped_file_with_path() was expecting a relative path" <--- that doesn't sound right

Sean (Jan 25 2020 at 21:51):

definitely seemed to run into a bug with the 'p' command -- I hit the same bug but didn't have time to debug it

Jeffrey Liu (Jan 25 2020 at 21:53):

I think you're right, I just took a look at the BUGS file and this may be the cause dsp primitives specifying data using Windows style file paths don't work. In which case, my bug ticket is wrong/already reported...

Sean (Jan 25 2020 at 21:53):

the 'p' parameter command is tailored to different states, so there's undoubtedly something just not described right for the state it needed to be in

Sean (Jan 25 2020 at 21:54):

Jeffrey Liu (Jan 25 2020 at 22:05):

I did some debugging on it back when I was working on the EBM tasks, and if I recall correctly, it was because of a conditional in mged_param() where es_edflag didn't satisfy the if statement so it jumped to the else statement for "three arguments needed"

Jeffrey Liu (Jan 25 2020 at 22:18):

Yeah, so the condition checks for this: if (es_edflag == PSCALE || es_edflag == SSCALE || es_edflag == ECMD_BOT_THICK), but the DSP and EBM settings are setting es_edflag to these:

#define ECMD_EBM_DSRC       53  /* set EBM data source */
#define ECMD_EBM_FSIZE      54  /* set EBM file size */
#define ECMD_EBM_HEIGHT     55  /* set EBM extrusion depth */

#define ECMD_DSP_FNAME      56  /* set DSP file name */
#define ECMD_DSP_FSIZE      57  /* set DSP file size */
#define ECMD_DSP_SCALE_X        58  /* Scale DSP x size */
#define ECMD_DSP_SCALE_Y        59  /* Scale DSP y size */
#define ECMD_DSP_SCALE_ALT      60  /* Scale DSP Altitude size */

Jeffrey Liu (Jan 25 2020 at 22:20):

An easy fix would be to just add the correct settings to the conditional (I think that's what's already being done for ECMD_BOT_THICK). I could do that right now, unless there's a better approach?

Jeffrey Liu (Jan 25 2020 at 22:46):

Taking a bit of a closer look, I'm wondering if the conditional should actually be checking for something like EDIT_SCALE instead, which seems to encompass more things instead of just PSCALE AND SSCALE, although it still doesn't account for EBM or DSP's FSIZE.

Jeffrey Liu (Jan 25 2020 at 23:31):

This change worked for me p_command_fix.patch, if I'm using EDIT_SCALE as it was intended. Setting EBM file size and depth and DSP altitude work as expected with the change applied.

Jeffrey Liu (Jan 28 2020 at 06:36):

I have been getting another error, but I've had some trouble debugging it because I think it's TCL related... for me, I'm consistently getting an error when I try to modify/create a DSP using the primitive editor. Curious if other people have seen this?

Jeffrey Liu (Jan 28 2020 at 06:58):

These are the errors I'm getting - they are when I try to edit or create a DSP respectively: error.log error1.log

Sean (Feb 03 2020 at 17:42):

Jeffrey Liu (Feb 04 2020 at 04:54):

Sean (Feb 04 2020 at 05:00):

The edsol code is just awful but your added case for dsp and ebm looked good to me.

Sean (Feb 04 2020 at 05:01):

Jeffrey Liu (Feb 04 2020 at 05:02):

I noticed that the conditionals for PSCALE and SSCALE weren't covering what was needed for DSP and EBM, which is why I changed it to EDIT_SCALE which seemed to include it. It may not be the correct macro to use in that case though?

Sean (Feb 04 2020 at 05:04):

Sean (Feb 04 2020 at 05:05):

it seems okay on the surface. question would be whether any of the additional clauses covered by, say SEDIT_SCALE, supposed to take more than one parameter

Sean (Feb 04 2020 at 05:05):

Jeffrey Liu (Feb 04 2020 at 05:08):

I'm not sure, I'll take a look at it again to make sure. There were some other options with SEDIT_SCALE and OEDIT_SCALE that I'd never used before, so it's possible that there are some that take in more than a single parameter.

Sean (Feb 04 2020 at 05:08):

that would be the only thing to make sure, since it adds about 10 more that wheren't being tested there

Jeffrey Liu (Feb 04 2020 at 05:09):

Jeffrey Liu (Feb 04 2020 at 05:19):

I'm pretty sure I tested the DSP and EBM scaling when I changed it, but I didn't really find much documentation on some of the other things (Cline?) so I couldn't test those

Sean (Feb 04 2020 at 05:20):

Jeffrey Liu (Feb 04 2020 at 05:23):

Sean (Feb 04 2020 at 07:20):

probably ignorable, I think they're all RT_NURB_ which is the old nurbs implementation, now defunct

Sean (Feb 04 2020 at 07:20):

Jeffrey Liu (Feb 04 2020 at 18:48):

Aside from those, the rest of them (besides EBM and DSP), if I remember correctly, had to do with VOL. I should be able to test those myself though.

Jeffrey Liu (Feb 04 2020 at 20:59):

Just saw that the EBM changes were implemented! One minor thing when merging though, I might have noticed and changed this after I submitted the original patch. Sorry about that! ebm_src.patch

Jeffrey Liu (Feb 04 2020 at 21:00):

For reference, it's just that ebm->datasrc was being set to RT_DSP_SRC_(TYPE) instead of RT_EBM_SRC_(TYPE). My fault - I probably changed this later but forgot to update the patch.

Sean (Feb 05 2020 at 16:58):

Sean (Feb 05 2020 at 17:05):

@Jeffrey Liu since you've been working in that area of the code and have some experience now, and see how VOL is pretty much in the same situation as EBM and DSP, can you think of a good way to generalize what they're doing in a reusable manner

Sean (Feb 05 2020 at 17:06):

so that VOL isn't version #3 of the get_file_data() and get_obj_data() .. and whatever additional objects that end up supporting internal and external data sources

Jeffrey Liu (Feb 05 2020 at 20:04):

Interesting, like get_obj_data() and get_file_data() in a separate file that can be called by any of the three?

Sean (Feb 05 2020 at 21:16):

Sean (Feb 05 2020 at 21:23):

I'm thinking maybe something like a "struct rt_data_obj *data" field that would replace the existing name field, src field, and the #defines for src/obj.

Sean (Feb 05 2020 at 21:24):

Maybe something like a simple api that works with it like rt_data_read(struct rt_data_obj , (rt_data_file_func)(), (*rt_data_obj_func)()) and rt_data_write() to read and write object data using callback functions defined by individual primitives. Or alternatively, something involving ICV to just specify an image/data source and let the primitive's prep auto-convert accordingly.

Jeffrey Liu (Feb 06 2020 at 01:45):

Hm I didn't even think of the struct, but that definitely seems a lot cleaner. I'm not sure if I completely understand what you mean by the API though, could you elaborate?

Jeffrey Liu (Sep 29 2020 at 22:36):

@Sean if I remember correctly, you commented on one of my tickets recently about the EBM GUI. I'm not sure if you saw my response, but I didn't commit those changes. I uploaded them to another patch here: https://sourceforge.net/p/brlcad/patches/540/. Please let me know if there's anything else I can do to help!

Stream: brlcad

Topic: EBM

Jeffrey Liu (Dec 23 2019 at 17:47):

Jeffrey Liu (Dec 23 2019 at 17:50):

Jeffrey Liu (Dec 23 2019 at 17:52):

Jeffrey Liu (Dec 24 2019 at 06:33):

Jeffrey Liu (Dec 24 2019 at 06:34):

Jeffrey Liu (Dec 24 2019 at 06:37):

Sean (Dec 24 2019 at 06:39):

Jeffrey Liu (Dec 24 2019 at 06:40):

Sean (Dec 24 2019 at 06:40):

Sean (Dec 24 2019 at 06:42):

Jeffrey Liu (Dec 24 2019 at 06:44):

Sean (Dec 24 2019 at 06:44):

Sean (Dec 24 2019 at 06:45):

Sean (Dec 24 2019 at 06:45):

Jeffrey Liu (Dec 24 2019 at 06:46):

Jeffrey Liu (Dec 24 2019 at 06:47):

Sean (Dec 24 2019 at 06:54):

Sumagna Das (Dec 24 2019 at 06:55):

Sean (Dec 24 2019 at 06:55):

Sean (Dec 24 2019 at 06:55):

Sumagna Das (Dec 24 2019 at 06:55):

Jeffrey Liu (Dec 24 2019 at 06:56):

Sean (Dec 24 2019 at 06:57):

Jeffrey Liu (Dec 24 2019 at 06:58):

Sean (Dec 24 2019 at 06:58):

Sean (Dec 24 2019 at 06:58):

Jeffrey Liu (Dec 24 2019 at 07:00):

Jeffrey Liu (Dec 24 2019 at 07:22):

Sean (Dec 24 2019 at 07:23):

Sean (Dec 24 2019 at 07:23):

Sean (Dec 24 2019 at 07:24):

Sean (Dec 24 2019 at 07:24):

Sean (Dec 24 2019 at 07:24):

Jeffrey Liu (Dec 24 2019 at 07:25):

Sean (Dec 24 2019 at 07:25):

Sean (Dec 24 2019 at 07:26):

Sean (Dec 24 2019 at 07:26):

Sean (Dec 24 2019 at 07:26):

Sean (Dec 24 2019 at 07:28):

Sean (Dec 24 2019 at 07:28):

Jeffrey Liu (Dec 24 2019 at 07:28):

Sean (Dec 24 2019 at 07:30):

Jeffrey Liu (Dec 24 2019 at 07:31):

Sean (Dec 24 2019 at 07:32):

Sean (Dec 24 2019 at 07:33):

Sean (Dec 24 2019 at 07:34):

Sean (Dec 24 2019 at 07:34):

Jeffrey Liu (Dec 24 2019 at 07:34):

Jeffrey Liu (Dec 25 2019 at 09:37):

Jeffrey Liu (Dec 25 2019 at 09:39):

Jeffrey Liu (Dec 25 2019 at 09:44):

Jeffrey Liu (Dec 25 2019 at 18:14):

Jeffrey Liu (Dec 25 2019 at 18:18):

Thusal Ranawaka (Dec 26 2019 at 01:02):

starseeker (Dec 26 2019 at 01:37):

Jeffrey Liu (Dec 26 2019 at 01:40):

Jeffrey Liu (Dec 26 2019 at 01:50):

Jeffrey Liu (Dec 26 2019 at 01:51):

starseeker (Dec 26 2019 at 03:08):

starseeker (Dec 26 2019 at 03:09):

starseeker (Dec 26 2019 at 03:18):

Jeffrey Liu (Dec 26 2019 at 03:49):

starseeker (Dec 26 2019 at 08:54):

Jeffrey Liu (Dec 27 2019 at 04:30):

Jeffrey Liu (Dec 27 2019 at 04:32):

Jeffrey Liu (Dec 27 2019 at 04:46):

Jeffrey Liu (Dec 27 2019 at 08:37):

Jeffrey Liu (Dec 27 2019 at 08:37):

starseeker (Dec 27 2019 at 13:17):

starseeker (Dec 27 2019 at 13:18):

Thusal Ranawaka (Dec 27 2019 at 13:19):

starseeker (Dec 27 2019 at 13:19):

Thusal Ranawaka (Dec 27 2019 at 13:20):

starseeker (Dec 27 2019 at 13:20):

starseeker (Dec 27 2019 at 13:24):

Jeffrey Liu (Dec 27 2019 at 17:14):

Jeffrey Liu (Dec 27 2019 at 17:44):

Jeffrey Liu (Dec 27 2019 at 17:48):