great @Sharan Narayan .. i'll see if i can review that page soon. as for the check command, you currently have to run the check.sh script first, then you provide the log file it produces to the check command
Thanks that worked. I got a .overlaps file which I fed to check.tcl in mged.
Now I need to think about the new functional GUI changes I can bring to this tool.
As of now I am going through the Tcl/Tk language and familiarizing myself with it. Any suggestions that I should keep in mind?
nothing comes to mind other than we really want this interface to be as simple, automatic, and general as possible. as to the code, tcl/tk is a relatively simple language. be sure to check the TODO file in src/tclscripts/checker as it has all the past thoughts on what to do next.
Yeah I saw the TODO list. Tcl/Tk is simple as you said. But it uses itcl and itk. I'm gonna learn that now. When proposing for the idea I'll make sure to attach some prototype screenshots of what I hope to make :simple_smile: also keeping the TODO in mind.
After going through the TODO list, there are a few GUI and a few performance/efficiency related problems. Since the project is revamping the GUI of the overlap tool, should I only consider the GUI related problems? or think about the performance problems as well?
By performance problems, I mean issues like Faster check command, Better overlap detection, etc.
And where would I post the proposal for preview before the actual submissions begin?
@Sharan Narayan if you're going to propose working on the overlap tool, performance is not a primary concern. the biggest gap there is converting what is currently implemented in a shell script into code that will run cross-platform (either in C/C++ or Tcl). you certainly could propose better overlap detection, but that's almost a project in itself. regardless, you will need to be familiar with the existing overlap tools (gqa, rtcheck, and the overlap GUI).
you can post a preview anywhere that is convenient -- our wiki, google doc, etc
@Sean Thanks for the input, I have put together the proposal on google docs, please provide any comments or feedbacks.
https://docs.google.com/document/d/15vXuiRSQFjkswHcswRKEV3hW-kzvoyTvPN-BbwBdzz0/edit?usp=sharing
@Sharan Narayan thanks for sharing the write-up, looks like some good progress. Did you read over the overlap tool TODO file?
@Sharan Narayan thanks for sharing the write-up, looks like some good progress. Did you read over the overlap tool TODO file?
Yes I did read the TODO file in src/tclscripts/checker folder. I hope that's what you meant by overlap tool TODO.
I have got the list of issues and ideas from there only but there are a lot of things that I didn't understand from the file.
I have made a docs file with my doubts about the TODO file.
https://docs.google.com/document/d/1N3Enik_DhtmdHX6yynO0vg8rdjyiVTji2EA7HSjrLBo/edit?usp=sharing
Please add comments to that file so that they are clear for me, Thanks.
Thanks @Sharan Narayan will take a look and see -- have you had a chance to create and resolve overlaps using the current tools (like gqa or rtcheck) and the current gui?
there's a lot of terminology that cannot be easily explained until you actually get some experience with what overlaps are, and how they are dealt with
I had used the rtcheck command back in the GCI days. Which displayed the overlaps in yellow lines according to the perspective we are currently viewing ( az, el values). I still have to use gqa. I'll read the man page on how to use it. I will also try out the current GUI in MGED.
Will ask for help. If anything comes up.
Thanks @Sharan Narayan will take a look and see -- have you had a chance to create and resolve overlaps using the current tools (like gqa or rtcheck) and the current gui?
Thanks for the feedback on the TODO file.
Sorry for the late response, had my exams. Today was the last one :simple_smile: , so back on track.
I have gone through it and did some research and made a few changes to it.
There are still a few things I don't understand like: bboxes, cyclic hierarchies, cycles,matrices and ORCA geometry.
I tried finding any relevant documentations related to these but couldn’t find any..
Please have a look at the changes I made and the remaining doubts, Thank you.
https://docs.google.com/document/d/1N3Enik_DhtmdHX6yynO0vg8rdjyiVTji2EA7HSjrLBo/edit
@Sharan Narayan I talked with one of the other devs and I think it will be best if the focus of any work on the overlap tool was towards implementing it in C or C++ instead of new features or GUI work (other than reworking the GUI to call whatever new backend.
you're going to have trouble finding documentation because those are complex topics that are spread across multiple documents and are generally learned from hands-on modeling experience
see the bb command for bboxes; you can create a cyclic hierarchy in mged with the c, comb, or g commands (and probably many others); a cycle is a cyclic hierarchy; matrices are a fundamental modeling concept learned by doing -- do all the mged tutorials then the oed tutorial to understand; orca geometry is just geometry with unpushed matrices.
@Sharan Narayan I talked with one of the other devs and I think it will be best if the focus of any work on the overlap tool was towards implementing it in C or C++ instead of new features or GUI work (other than reworking the GUI to call whatever new backend.
That means I should remove everything related revamping the GUI ? from my proposal and focus more on the cross compatibility part.
I will rewrite the proposal and include the details about the initial GUI that should appear to provide arguments for creating the .overlap file.
if you implement something clean in c/c++, there will be no need for writing an overlap file ... it'll be in memory and can be queried by a gui
if you implement something clean in c/c++, there will be no need for writing an overlap file ... it'll be in memory and can be queried by a gui
Using C/C++ for processing outputs of commands like rtcheck would be difficult.
Can I use any external libraries to do string processing? like Boost for C++.
@Sean
Nevermind that. I managed to parse the rtcheck and gqa logs without using awk, sed and cut.
Using C's fscanf function and scanset formatting, I extracted the information from the files.
we don't want to add boost as a dependency if we don't have to -- there are a lot of string processing functions in libbu (and it's easy enough to add more as needed).
that said, we also don't really want to be doing string processing ultimately, because that means any change to one tool will break the other. it should be working with the data in binary, keeping it in that form until it's time to print.
that said, we also don't really want to be doing string processing ultimately, because that means any change to one tool will break the other. it should be working with the data in binary, keeping it in that form until it's time to print.
Okay, but how do I get just the overlaps from the rtcheck command or gqa command?
I had to use text processing because the output logs from these files don't give the overlaps directly.
using text processing is okay in the strict sense of that's what the current code is doing, but you'll want to keep it as modular as possible so that it can be replaced with overlaps from some binary interface (which doesn't exist)
you'd get the list by ... implementing something that gets them, and making rtcheck/gqa/check use that new interface
a plan would probably be something like refactoring rtcheck into a libanalyze function, then refactoring rtcheck to use the new function, then adding the parallel capabilities of gqa into the libanalyze function, then refactoring gqa to use it, then adding your new command in libged that also uses that function (doing the work of both rtcheck and gqa)
a plan would probably be something like refactoring rtcheck into a libanalyze function, then refactoring rtcheck to use the new function, then adding the parallel capabilities of gqa into the libanalyze function, then refactoring gqa to use it, then adding your new command in libged that also uses that function (doing the work of both rtcheck and gqa)
This is what I understood from it.
To implement a function in rtcheck that does not affect the current functionality of rtcheck, but returns the output as a list, which can be processed by the check command. And same for the gqa command.
Finally adding the command to libged.
Please correct me if I am wrong. Thanks
you almost said what I said .. and the places where you said something different aren't ideal (e.g., you said "implement a function in rtcheck" -- I said implement a function in libanalyze)
After going through rtcheck.c, I am not able to understand any of it. There are so many functions and data structures which are BRLCAD specific. To make sense of it, I thought I would check the header files. Same story there, the header files are depending on other header files.
Any suggestions ? :/ Without understanding them, I am stuck.
@Sharan Narayan it sounds like you don't have that much experience reading code ... and that is an essential skill to work on as a developer. what that means is the goals we've discussed are quite possibly beyond your abilities and you may need to consider proposing something different or spend a LOT more time studying the code
one thing that might help is to read through these, particularly the first one since it speaks to the structure of the rtcheck application (and other rt* applications and gqa): http://brlcad.org/wiki/Developing_applications
if you still don't understand after studying those materials and looking again at the code, that would be a good time to re-plan
Hey @Sean , Glad to say I made some progress and understood most of the rtcheck.c file.
read the linux implementation of rtcheck.c
Understood how tclfilehandler calls the functions for stdin and stderr, stdin is used for the output as plot3 file. stderr is the logs that get printed.
But there are some things which I didn't understand which I have linked to this doc file,:
https://docs.google.com/document/d/1NTftagw4Ie7e_PgmF400dWPOAGHZOZEj5126fTh_qQw/edit?usp=sharing
Please give some feedbacks, Thank you.
@Sharan Narayan I'm glad you've made some progress, but you've not understood it enough to realize that file is just a wrapper to the real rtcheck application... the source code to the "rtcheck" application is not in rtcheck.c -- that's just the mged command which performs an exec of the real app. all the questions you have are about how that wrapper works, which is rather irrelevant to the project you've mentioned. you're going to need to spend many weeks researching and exploring like you've just done to understand your project's needs. I want to be supportive but your proposal must be realistic. make sure you don't propose doing too much.
I would suggest simplifying your proposal objectives and then shifting efforts onto a coding task, try to make a patch that does something simple like create a new mged command that just prints hello.
or fix some related libged/rtcheck/gqa issue in our TODO or BUGS file
Thank you for the feedback
the source code to the "rtcheck" application is not in rtcheck.c -- that's just the mged command which performs an exec of the real app.
Haha, just what I thought, that is why it confused me, that rtcheck is calling itself. Little did I know command is defined somewhere else.
you're going to need to spend many weeks researching and exploring like you've just done to understand your project's needs.
Yup, that is why I decided to learn how this works before proposing anything. So that I could evaluate myself, because proposing something that I cannot do is risky. The problem is I can't devote much time for that with my busy schedule of college. I believe, I would have more time during my holidays to go through the code thoroughly.
I want to be supportive but your proposal must be realistic. make sure you don't propose doing too much.
But after spending many days with this topic, I am really excited to work on it. So I don't want to propose any less than what we have discussed so far, because everything is crucial for the completion of the project.
I would suggest simplifying your proposal objectives and then shifting efforts onto a coding task, try to make a patch that does something simple like create a new mged command that just prints hello.
or fix some related libged/rtcheck/gqa issue in our TODO or BUGS file
Yes, I will complete my proposal before its late and focus on making a patch for something simple.
One quick question, Can I submit the patch later and link it here in case I can't include that in my proposal?
@Sean As instructed on the Deadline Soon topic. I have submitted my final PDF. But please have a look at the draft and let me know if there is anything that needs to be changed. Thanks
I had submitted a patch regarding tables.c. it's unrelated to this project. But was under libged and seemed easy so I did it anyway.
Hey @Sean ,
I had posted a different design approach for the check command in my final proposal. I sure hope it was good.
Hence I had been doing a couple of Tk tutorials and examples in the last few days. Should I rather spend this time concentrating more on submitting patches fixing bugs and to-do ?
Just submitted a patch, implementing a libbn function to support angles input in degrees and radians
Hey @Daniel Rossberg..
Glad to know you are my mentor for the project :). I will follow the checklist as said by Sean.
I had this one thing to let you know, my end semester exams got postponed to 26th April to 17th May 2018. Previously it was from 23rd April to 8th May. Nonetheless I will manage by allocating time judiciously.
Hi @Saran Narayan , congratulations for being selected! And, these 3 days should me manageable, with a strong community bounding period, for example.
Hi @Saran Narayan , congratulations for being selected! And, these 3 days should me manageable, with a strong community bounding period, for example.
Thank you Daniel. Yeah, I am sure I can manage since I have good gaps between each exam.
I added my profile here : http://brlcad.org/wiki/Google_Summer_of_Code/2018
Also posted the proposal, I will add the milestones in the coming days.
I had this doubt regarding the 'agreement to the acceptance requirements'. I read that it must be done in writing. Can someone elaborate and where should this proof of agreement be uploaded or sent?
Hey @Shubham Rathore Thanks a ton!
Yea sure thing, it'll be up and updated soon.
Would you recommend hosting the devlogs on BRL-CAD's website or would you recommend using an external page (maybe a blog or a github.io page)
I'm on the verge of completion of my sessionals at university in a couple of days. I'll start work on formalizing the project, latest by the 1st of May. :)
@Daniel Rossberg
I was trying to understand how calling rtcheck command in MGED works and made a flowchart as follows:
rtcheck.jpg
What I don't understand is how does it reach viewcheck.c file. Because the viewcheck.c handles the overlaps and the list of overlaps.
@Saran Narayan you got it right all the way up to that last block
in rt's main(), it makes calls into the RTUIF "API", which is basically a way to say that it calls a set of named functions directly, which each rt-style application provides. So viewcheck.c defines a view_init for example, and a bunch of other rtuif-functions, which main directly calls (main.c line 382)
if you look at rtuif.h, you'll see the set of functions that must be defined for a given application
and you'll find all of those for rtcheck in viewcheck.c -- we call that the "backend" file whereas main.c and opt.c and such are the "frontend"
@Saran Narayan find it out by yourself - with the help of a debugger. I'll give you an example using the Gnu debugger gdb
.
You already found out that the program which hast to be examined is rtcheck
. Therefore, start the debugger for this program:
gdb rtcheck
Then tell the debugger which function you want to examine and set a break point there. In your case it's some function from viewcheck.c, e.g.:
break overlap
Next, run the rtcheck program inside the debugger with some useful parameters, e.g.:
run -g10 -G10 m35.g all.g > m35.plot3
After some seconds, the program execution will stop at the desired break point. Now, you can ask for the backtrace, i.e. the list of functions on the call stack:
bt
in rt's main(), it makes calls into the RTUIF "API", which is basically a way to say that it calls a set of named functions directly, which each rt-style application provides. So viewcheck.c defines a view_init for example, and a bunch of other rtuif-functions, which main directly calls (main.c line 382)
Yes, I did read about this in the 'application_development.pdf'
and you'll find all of those for rtcheck in viewcheck.c -- we call that the "backend" file whereas main.c and opt.c and such are the "frontend"
That explains why it was written second half in the beginning of viewcheck.c.
@Saran Narayan find it out by yourself - with the help of a debugger. I'll give you an example using the Gnu debugger
gdb
.
Thanks for this tip! gdb is amazing.
It did help a lot, but it showed a final jump from librt/bool.c to viewcheck.c. From a brief reading of bool.c, I think bool.c does the actual checking of the overlap. As @Sean said I will go through main.c and also check bool.c in detail.
Okay I think it's making sense now.
In bool.c at the end when it jumped to viewcheck.c there is a call to ap->a_overlap( )
view_init( )
in viewcheck.c initializes ap->a_overlap = overlap
where overlap its the local function overlap
in the viewcheck.c.
but how does main.c know that it has to call the view_init( )
of viewcheck.c only? Is it the way the linking is performed for rtcheck while compilation happens ?
Yeah I checked the CMakeLists and for rtcheck, viewcheck.c is there along with other librtuif sources :)
Yep, you got it. It only links one view file.
That way we can (and do) have severa rt-style applications that all use the exact same front end code, recompiled for each one, with each also linking their respective view file.
The utilization of function pointers allows generic programming in C. You can read more about how to utilize the BRL-CAD ray trace functionality with the application struct here: https://brlcad.org/wiki/Developing_applications
The utilization of function pointers allows generic programming in C. You can read more about how to utilize the BRL-CAD ray trace functionality with the application struct here: https://brlcad.org/wiki/Developing_applications
Thank you, I will check that! BTW this is my progress on rtcheck, Please let me know if there is anything missing. I created with some rough assumption without understanding most of it! :D should I be knowing everything that going on there?
rtcheck3.jpg
I will also go through gqa
in this manner. Then I will try thinking about the project plan and discuss here :)
@Saran Narayan The rtcheck program is a BRL-CAD application of the kind described in the referenced documents: The application struct is prepared, the grid is set up, (librt is set up for parallel execution,) and then rt_shootray() will be called.
@Daniel Rossberg Yes I got that from the PDF and the sample program :simple_smile:
Regarding the project, I had some questions like:
What are libanalyse functions generally used for ?
For getting the list of overlaps I was suggested by @Sean to refactor the rtcheck command into a libanalyse function, then refactoring rtcheck to use the libanalyse function.
What I had thought and initially planned was to call the libanalyse function that will handle the overlaps in place of the viewcheck's overlap()
. When rtcheck is called by MGED or as an application the execution is normal but when called via check
command the overlap list is handed over to the check
command along with the normal execution ( with some flags to denote that check
is caller ). I somehow feel like this is not gonna work or is the wrong way to do it.
The hard part is getting the list in a 'right' way. After that processing the list like removing duplicates and sorting should be straight forward.
libanalyze is a relatively new library and can/should be changed as tools are refactored
consider the current situation with most of the rt* applications using the "rtuif" code in src/rt to set up a grid of rays, shoot them, and then use their view*.c file to post-process them.
that's fine for single grids but then gqa comes along using a completely different method (shooting 3 grids iteratively)
there's obviously a lot of overlap (i.e., needs to shoot sets of rays), but adapting rtuif to gqa didn't make much sense as gqa is essentially view-agnostic and rtuif is specifically view-centric ... so then there was an idea to extract the common reusable parts into functions that both gqa and rtuif applications could use
and the place to put that function or those functions ... libanalyze
conceptually, libanalyze is intended to be where all "higher-level" volumetric geometry analysis occurs -- e.g., detecting overlaps, comparing for differences, moments of inertia, masses, volumes, etc.
it's not yet undergone any API design, so you're welcome to try designing what you need and I'd be glad to help you with that -- or just focus on exactly what you need for your project, that's fine too
Thank you for the explanation about the libanalyse functions and the origins of it.
it's not yet undergone any API design, so you're welcome to try designing what you need and I'd be glad to help you with that -- or just focus on exactly what you need for your project, that's fine too
I think right now for GSoC, I think I should be just focusing on the project. After completion of the GSoC project I will be more than happy to discuss regarding the API design.
Hey @Daniel Rossberg,
I needed your opinion on this :
What I had thought and initially planned was to call the libanalyse function that will handle the overlaps in place of the viewcheck's
overlap()
. When rtcheck is called by MGED or as an application the execution is normal but when called viacheck
command the overlap list is handed over to thecheck
command along with the normal execution ( with some flags to denote thatcheck
is caller ).
I hoped this would work because the way linking is done for rtcheck command all the variables are accessible from main.c. So if linked similarly I would have access to the viewcheck's local overlap list.
Also instead of replacing viewcheck's overlap(), I could add a function call to a libanalyse function that would append the list to a linkedlist locally available in libanalyse file. Finally my check command will free this new linked list after it's done processing it.
This way I would be able to write a common append function for both gqa and rtcheck.
The other way that came to my mind is to make a libanalyse function for rtcheck() which can be called by both check
and the normal rt application by refactoring it use the new function. This rtcheck() function would do all the work the rt/main.c will do.
This way would involve a lot of code duplication ( I believe so, correct me if I am wrong :) )
So which way would be best and feasible or is there any other way to it?
@Saran Narayan I'm not sure if I understand you right. I try to go therefore step by step:
First, you are right with the many global variables in the current rtcheck. These have to be eliminated when the functionality moves to libanalyze. A cheap method for a first step would be to put all these variables in a resource structure and have this structure as additional parameter for all relevant functions.
Doing so, it would be relatively easy to move these functions from rt to libanalyze, which would be a code duplication first.
You need then a method to separate the algorithm from the way the results are used. libanalyze will contain the algorithmic part then and the presentation part will be implemented in the library which wants to do the overlap analysis, i.e. for example libged or rt. This separation can be done with function pointers, similar to the application struct in rt_shootray(). Please, have a look at the voxelize() function in src/libanalyze/voxels.c which gives you an example. There, create_boxes is the function pointer parameter which determines the prozessing of the algorithm's output. Utilizations of voxelize() can be found in src/libged/voxelize.c and src/conv/g-voxel.c.
Thanks for the input!
Please, have a look at the voxelize() function in src/libanalyze/voxels.c which gives you an example. There, create_boxes is the function pointer parameter which determines the prozessing of the algorithm's output. Utilizations of voxelize() can be found in src/libged/voxelize.c and src/conv/g-voxel.c.
Yeah, I will have a look at it. I still have some doubts :/ maybe the example will help me.
@Daniel Rossberg
okay I read that but since rtcheck is a bit complex I am still having some concerns.
I had this idea on refactoring, Calling the the libanaylse function from ged_rtcheck
. Then from libanalyse executing the rtcheck
application using execvp()
. This would make the execution go as it is now but at the end it will return to the libanalyse function. But the viewcheck.c
frees the memory of the overlap list, to fix this I could do the freeing in my libanalyse function after appending it to a new local overlap list.
But later I realized that if I do that its only applicable for calling rtcheck
via MGED. But when I call it from terminal it would directly call the main
in rt/main.c
this would cause issues because then viewcheck.c won't be freeing the overlap list.
Since that wouldn't work, there is need to make the libanalyse independent and leave the executable rtcheck
application as it is.
To make it independent I would require do all the things going on in rt/main.c
again in my libanalyse function. This is why I asked about code duplication.
@Saran Narayan
I had this idea on refactoring, Calling the the libanaylse function from
ged_rtcheck
. Then from libanalyse executing thertcheck
application usingexecvp()
. This would make the execution go as it is now but at the end it will return to the libanalyse function.
I'm afraid, execvp() won't return to the process it was started from. rtcheck will end usually with a call to exit() which terminates the whole process.
Anyway, the calling of subprograms in BRL-CAD functions is something which we want to get rid of. It disturbs the program flow and has dependencies to the operating system which makes it more difficult to program OS independent. I'm afraid therefore that you need to understand how rtcheck works.
viewcheck.c is very straight forward: hit() - do nothing, miss() - do nothing, overlap() is the function which implements the processing of the results, and you probable need something similar as a function pointer parameter for the libanalyze function. And then, there is the output of the result.
Next, there are the rt general purpose functions as e.g. the do_~(). To get familiar with them you could copy all relevant source files in a new directory (e.g. src/libanalyze/overlap), remove all functions and features not needed by rtcheck, and make them compile. The should give you a much smaller code to understand as the current in src/rt.
Finally, you know something about how rtcheck should work. It shoots grids of rays with -g width and -G height, etc.. Go through the rtcheck parameters and see where and how there are used.
At the end, rtcheck should use the libanalyze function and not the ones from rt.
@Daniel Rossberg
Thanks for the reply! That clears most of my doubts.
Next, there are the rt general purpose functions as e.g. the do_~(). To get familiar with them you could copy all relevant source files in a new directory (e.g. src/libanalyze/overlap), remove all functions and features not needed by rtcheck, and make them compile. The should give you a much smaller code to understand as the current in src/rt.
I will try this tonight and see how it goes! I guess trying it out would help me the most rather than thinking of ideas :D
@Daniel Rossberg
I built a new version of the rtcheck command :)
I had commented out many things in main.c and other files which weren't related to rtcheck. Like statements using the options which rtcheck doesn't use. I believe I could remove out more content. Will do it soon, gonna sleep now.
BTW I did the following edits in CMakeLists. Not sure if what I did was proper because this was new to me.
changes.patch
Sounds like you are making progress :)
Your changes to CMakeLists.txt look reasonable. The only issue I found is that overlap/main.c shouldn't belong to CMAKEFILES.
@Daniel Rossberg
Please give your feedback about this plan.
After going through the example you suggested, i.e libged/voxelize.c and libanalyze/voxels.c
Voxelize.c - libged
createboxes()
which gets the callbackdata(voxData) from the libanalyze funciton.voxelize()
passing the pointer the voxData and pointer to the createboxes()
Voxels.c - libanalyze
rt_prep()
RT_APPLICATION_INIT()
rt_shootray()
createboxes()
with the pointer to the structure which was passed to libanalyzeI similarly planned the same for rtcheck
check.c - libged
rtchk()
with pointer to this struct, args for rtcheck command and pointer to the overlap function in check.coverlap()
and viewend()
functions.rtcheck.c - libanalyze
main()
of rt/main.c as rtchk()
that accepts the argc, argv, pointer to overlap struct and pointer to overlap functiongetargs()
from rt/opt.c to parse the argumentsoverlap()
in libgedged_rtcheck
will be using the libanalyze function similarly like the check.c but will have the original rt viewend()
and overlap()
@Saran Narayan
Because you emphasized the calBackData, let me explain that this structure is a simple pointer to the context where the call back function was defined. If you want for example fill a list, the call back function hast to get access to this list. To achieve this you could declare a global variable for this list or carry a pointer to the call back function which point to its original context with this list. (This "context" is a structure with pointers to all necessary data.)
How can this be transferred to the overlap feature?
You need an analyze_overlaps()
function in libanalyze (I wouldn't call this function "rtcheck"). This function takes parameters similar to the ones of the rtcheck program - and a call back function with its context. These call back function with its context could be identical with struct application.a_overlap
and struct application.a_uptr
, but it could be advisable to have an adapter between them.
Then e.g. in libged's check, you define an overlap list structure, a function which adds entries to this list, and hand it over to analyze_overlaps() together with the other parameters. When this function has finished your overlap list is filled and you can transfer it to the output. No viewend() any more!
The new analyze_overlaps() is the front end of most of the code from rt, this is true, but this is all hidden behind this single entry point.
@Daniel Rossberg Thank you for explaining it in detail, but I am afraid I didn't understand some of it.
The usage of contexts got me really confused.
libged - check.c
overlap()
as in viewcheck.c to add the entries to the list, call it add_overlaps()
ged_check()
calls the analyze_overlaps()
with args (like the ones of executable rtcheck), pointer to the add_overlap
function.add_overlap()
that is the struct overlapList. But since it is declared in the check.c wouldn't the add_overlap()
be able to access this list ? Were you mentioning it in the case if I don't have it declared globally?And there are variables like noverlaps, overlap_count and unique_overlap_count which are used by overlap()
in the original viewcheck.c (again defined globally) which are purely for printing purposes, which I don't need in my case.
In case of voxels I understood the need of passing the callbackData as voxDat is created in the local scope of ged_voxelize and create_boxes() actually used the data that was set by ged_voxelize.
- you mentioned the need to pass the context, which I believe would be the data required for the
add_overlap()
that is the struct overlapList. But since it is declared in the check.c wouldn't theadd_overlap()
be able to access this list ? Were you mentioning it in the case if I don't have it declared globally?
It's very bad practice to have or access globals in library code. It's not "great" for application code either, but it's not nearly as big a problem ... until you want to turn that application code into library code.
in general, if we're working around any code, application or library, it's generally good to eliminate globals where you can easily, especially if you're already in there working on that code
in fact for simply understanding the code, it might help you to eliminate globals one at a time, a patchfile each
some globals are super easy to eliminate, some require creating a context or some other structure to hold that data and then passing that around
Thank you @Sean that explains the need of contexts. Then I would have the variable of the overlapList in the local scope of ged_check() and call the analyze_overlaps() with args required, the pointer to the add_overlap function and the address of the variable overlapList.
So this way when everything is done executing I would have the results in my local variable.
I would require contexts in case of re-factoring the ged_rtcheck to this libanalyze function because then I would need to print these variables like the rtcheck application does.
And there are variables like noverlaps, overlap_count and unique_overlap_count which are used by
overlap()
in the original viewcheck.c (again defined globally) which are purely for printing purposes, which I don't need in my case.
And to pass the pointer the struct of context I should use the app.a_uptr because I cannot pass it as a function argument.
??
it depends ... if you're creating new API (in libanalyze), then you can do whatever you want including creating a user data pointer argument that allows it to be passed
hmm I am not sure what do u mean by the API, does it mean like writing your own functions instead of using the rt/librt ones?
yes, creating new functions vs using existing ones
"it depends"
Well, I would like to try to do the api, there is already some work done on it in libanalyze/api.c right?
But seeing myself struggle in understanding simple things so far, I am doubtful that it would set me off-schedule, So I think it would be safer for me to stick with using the existing ones :D
@Daniel Rossberg I am gonna try to implement what we discussed.
And about the interaction schedule, I would be free all the time during my break, so when would be comfortable for you? I am 3hrs 30mins ahead of your time according to zulip.
you just need to be aware where the existing functions are adequate or may be inadequate -- libanalyze is relatively new and hasn't had any design thought go into it.
only a few functions have been added where something was needed in a couple places
I agree that you seem to be missing some "simple things" as you put it, but that will come with some experience and reading
this might be a helpful tutorial: https://opensourceforu.com/2012/02/function-pointers-and-callbacks-in-c-an-odyssey/
this is also a good one that even speaks in terms of "contexts" which is just data being passed around: http://mindtribe.com/2015/06/do-c-callbacks-like-this-not-like-that/
Thank you so much for the links and being supportive :) I will go through them!
Then maybe read the current implementation of api.c and find what it is missing.
Don't stick on the api.c file, A functions isn't an API function because it is defined in a file called api.*.
Creating an API function means to write a function which is for use outside the library, in this case libanalyze. If you write a analyze_overlaps() function which shall be called in libged you are writing a libanalyze API function.
Ideally such a function would be defined in a canonical file like src/libanalyze/overlaps.c
As a starting point you could choose a behavior based approach. This means to think about how an analyze_overlaps() functions ideally should look like, which parameters it should have. Grid sizes, angles, objects, generally the rtcheck parameters.
If you have done this you can fill in the rtcheck code as the body of analyze_overlaps() and you are done :wink:
And about the interaction schedule, I would be free all the time during my break, so when would be comfortable for you? I am 3hrs 30mins ahead of your time according to zulip.
Let's say, I'm usually available around 17:00 UTC, but it's hard to keep a strict schedule.
Let's say, I'm usually available around 17:00 UTC, but it's hard to keep a strict schedule.
Okay sure!
As a starting point you could choose a behavior based approach. This means to think about how an analyze_overlaps() functions ideally should look like, which parameters it should have. Grid sizes, angles, objects, generally the rtcheck parameters.
If you have done this you can fill in the rtcheck code as the body of analyze_overlaps() and you are done :wink:
Thanks for the explanation, I will need some thinking before I can say anything on this. Because right now what you say seems just what we had planned so far except to pass the parameters explicitly.
Don't fear to code, express your ideas in code and share this code. Ask questions as you are doing here and share your code which is as same important.
Will do! :) next time I ask something I will share the code as well.
Just a quick update:
I had my exam yesterday so I couldn't do much. I will post my progress tonight after working on it.
So far I have added the check command in MGED and compiled it, right now it only prints the args. Was testing if the args are properly sent through because I was using the cmd_ged_plain_wrapper.
@Daniel Rossberg
For start I just tried the callback functionality with a int. After compiling, it worked and printed 345. I am attaching the patch file. test_callback.patch
Gonna try adding more to the libanalyze/overlaps/overlap_check.c
I think I need to add definition of struct overlap_list inside analyze.h because I used it inside in the overlaps_context struct's definition.
@Saran Narayan
This is a nice peace of code which allows us to discuss the basic structure of the analyze_overlaps() API function. I like that you were able to omit everything which could block the view at the API design.
First some words of introduction: You have two contexts here (you can translate context as "data space"): The one of the libanalyze and the one of libged, or to be more precise, the one of analyze_overlaps() and the one of ged_check(). What distinguishes the date of these two functions?
analyze_overlaps() knows the algorithm which can find overlaps and its output, say "object x" overlaps with "object y" by "z mm".
ged_check() knows about analyze_overlaps() interface, which is declared in include/analyze.h, and that it wants to create a list of all overlaps.
Notice, ged_check() doesn't know about the algorithm internals, like the ray-trace, and analyze_overlaps() doesn't know about the list the ged command wants to create. Furthermore, libanalyze has to disclosure the piece of its data to libged which contains the overlap algorithm's output.
This works in the first place by delegating the processing from libanalyze to libged for the processing of the overlaps. But, when the processing moves from libanalyze to libged, libged needs its context (= data) back.
You did this with the struct overlaps_context
. The structure is okay, but declared at the wrong place. overlaps_context contains the data of ged_check() (the list, an int). That's why is has to be declared in libged (I would recommend src/libanalyze/overlaps/check_overlaps.c).
Now, let's express this in code, starting with analyze_overlaps(). The function has the usual parameters as cell width and height etc., plus a function pointer to the call back function and a pointer to the other library's data space:
analyze_overlaps(double cellWidth, double ceelHeight, ..., analyse_overlaps_callback callBack, void* callBackData);
Next, when analyze_overlaps() transfers the processing to ged_check() it needs to transfer some data too (see above for the example I chose), especially to the call back function:
typedef void (*analyse_overlaps_callback)(const char* object1, const char* object2, double distance, ..., void* callBackData);
This way libanalyse can publish its overlap interface in include/analyze.h and via callBackData the calling library may get its data back when the processing is transferred to its call back function.
I'm not sure how the overlap data really looks like. In principle it's the data the application.a_overlap function experiences. You may either transfer it as it is to analyse_overlaps_callback or compile it to better understandable terms.
Be aware that the above code my contain bugs as it is untested.
Thanks for the feedback and input. I will look into it and see if I am able to follow :)
@Daniel Rossberg
As you said that we would need to pass the usual parameters to the analyze_overlaps(), that would mean all the parameters of the rtcheck command right? so I added a parse_args() function to check.c to parse these args (ignoring the debug ones) to the following patch: edited will add a new one
I didn't add all the changes you mentioned because the list of parameters is a large list.
I was thinking if we could use a struct to pass the args, but then would that be okay if we are trying API?
While I was waiting, I decided to remove more code from front-end of rt that is not required for rtcheck and compiled it, so that it would easier for me to understand.
You use global variables in src/libged/check.c, this is bad. Consider to make the argument parsing part of ged_check() and the matflag etc. local variables there.
You use global variables in src/libged/check.c, this is bad. Consider to make the argument parsing part of ged_check() and the matflag etc. local variables there.
Yep that I will move to the ged_check()
Don't be afraid of a large parameter list. I's only a good idea to use a structure for them if many functions need them.
On the other hand, using a large parameter list is more secure then using a structure because the user is forced to think about all of them this way. In case of a structure there is a high danger that they forget one and may use the wrong default.
Don't be afraid of a large parameter list. I's only a good idea to use a structure for them if many functions need them.
On the other hand, using a large parameter list is more secure then using a structure because the user is forced to think about all of them this way. In case of a structure there is a high danger that they forget one and may use the wrong default.
yeah it's a good idea but 14 parameters sounded too much that's why I hesitated. Will write code to pass them.
BTW, for a simple test I would let callBackData be a pointer to an integer which counts the overlaps:
HIDDEN void count_overlaps(..., void* callBackData) { int* value = (int*)callBackData; value++; } int ged_check(...) { int numberOfOverlaps = 0; analyze_overlaps(...., count_overlaps, &numberOfOverlaps); print(numberOfOverlaps); }
I had this doubt like for the existing script only ran the rtcheck command with these :
$rtcheck -o $OBJ.$az.$el.plot3 -s $sz -a $az -e $el $DB $obj
that means only o,s,a and e are used. So the parsing of args is larger now than I require.
The analyse_overlaps takes all the arguments but ged_check() only would use the above mention args only
when I refactor rtcheck then I would require all the parameters.
yeah it's a good idea but 14 parameters sounded too much that's why I hesitated. Will write code to pass them.
Hmm, interesting.
These are fore sure analyze_overlaps() parameters:
These values can or must taken from the current view
Which one have I forgotten?
Thinking about this again, I would probably put the azimuth and elevation to the parameter list, the objects have to be selected before the call of analyze_overlaps(). In case of the mged command these objects are simply the currently displayed once. So, it behaves as one would expect it to behave.
Which one have I forgotten?
square grid size, read from matrix, number of processors, plot file and a few debug flags like librt, rt, NMG
Thinking about this again, I would probably put the azimuth and elevation to the parameter list, the objects have to be selected before the call of analyze_overlaps(). In case of the mged command these objects are simply the currently displayed once. So, it behaves as one would expect it to behave.
About the objects, I had planned to like give the objects list from GUI.
So it would be in argv
- square grid size: -s is only a way to give the width and hight in one value, or?
- read from matrix: there is no stdin, so, what is it?
- number of processors: okey
- plot file: this is a post processing feature which has to be implemented in the call back.
- a few debug flags like librt, rt, NMG: they had to be set before, no matter of analyze_overlaps()(?)
square grid size fine
read from matrix was there in the usage string for rtcheck. This because I had planned to refactor MGED's rtcheck, should I drop that plan ? :D
well in MGED we can't give the stdin so that won't be necessary
About the objects, I had planned to like give the objects list from GUI.
I'm not sure if it's possible to have more than one list of selected objects. If not, the command would change the object selection in mged which is undesirable.
However, in any case the objects had to be selected with rt_gettree() first before analyze_overlaps() can be called.
I'm not sure if it's possible to have more than one list of selected objects. If not, the command would change the object selection in mged which is undesirable.
The existing overlap tool does have a text box to input objects
but this list is filled automatically with displayed objects
The matrix (matflag) seems to define the view, similar to azimuth and elevation. Therefore, to allow a matrix description of the view analyze_overlaps() could have a double* parameter which points to the projection matrix. If the user gave azimuth and elevation then the corresponding projection matrix had to be determined first and then handed over to analyze_overlaps().
The existing overlap tool does have a text box to input objects
Yes but if I understood it correctly this tool calls another program where everything is possible, but if we want to stay in the same process ...
However, this doesn't influence the design and behavior of analyze_overlaps().
However, this doesn't influence the design and behavior of analyze_overlaps().
yep that is true
The matrix (matflag) seems to define the view, similar to azimuth and elevation. Therefore, to allow a matrix description of the view analyze_overlaps() could have a double* parameter which points to the projection matrix. If the user gave azimuth and elevation then the corresponding projection matrix had to be determined first and then handed over to analyze_overlaps().
This I am not very clear, where does the project matrix creation happen? I didnt't find it in code. Is it do_ae?
This I am not very clear, where does the project matrix creation happen? I didnt't find it in code. Is it do_ae?
do_ae() does it implicitly by setting a global (sigh) matrix. The matflag stuff is implemented in src/rt/main.c line 469 ff. It reads in commands there which should set the view.
This needs some considerations: Should analyze_overlaps() use the global view or a view direction from its parameter list?
I am not sure about the concept of view. I need to read up on it.
I am not sure about the concept of view. I need to read up on it.
In principle it's a matrix which transformed the coordinates (they will be multiplied with the matrix). This way e.g. you simply print the x and y coordinates of the result on the screen, the matrix takes care of the rest.
Still don't get it :/. How can I use -M ?
Here is what I know: Using the saveview command I could generate a script that gives the viewsize, orientation, and eyept. That is processed by the rt_read_cmd.
The matflag stuff is implemented in src/rt/main.c line 469 ff. It reads in commands there which should set the view.
on line 469 it calls the oldway. Is that the function you were talking about
on line 469 it calls the oldway. Is that the function you were talking about
??? It says "New way" at line 473.
Okay the newway uses the rt_cmdtab to execute the cm_* functions according to the script right?
Right. But if it's old or new, it's simply another way to describe a matrix. (Or, the direction from where you want to shoot the rays.)
oh so this matrix is used to define the direction of the ray to be shot using a matix.
This matrix is to be created with or without the -M option?
Yes, the default is the identity matrix (1s in the diagonal, 0s otherwise).
Where is this matrix information stored? model2view view2model?
For src/rt: Yes. These two are inverse of each other: model2view = view2model^-1
This needs some considerations: Should analyze_overlaps() use the global view or a view direction from its parameter list?
Okay back to this then. Since we need the view with/without the -M, libanalyse would be the place to generate this view in my opinion
Okay back to this then. Since we need the view with/without the -M, libanalyse would be the place to generate this view in my opinion
Sounds reasonable :)
When writing the libanalyze function. I need to write the parts of main, do and worker of src/rt.
but in rt there are a lot of global variables which makes it really easy for it to communicate
So when writing the libanalyze function would I use the same concept or like have one single file which does everything
I am gonna summarize today's discussion and understand what ged_check should do and pass as parameters to libanalyze.
parameters of the libanalyze:
ged_check:
according to what the current script is doing:
for obj in $tops ; do for az in `$loop 0 179 45` ; do for el in `$loop 0 179 45` ; do $rtcheck -o $OBJ.$az.$el.plot3 -s $sz -a $az -e $el $DB $obj 2> $OBJ.$az.$el.rtcheck.log
ged_check would get the list of objects as argument, using this list for each object it would call libanalyze in an iterative manner with az, el values changing, the other parameters have the default value.
For each run, we would get an overlaplist which has to be preserved in an object based list. After each object run, this object list needs to be preserved in a final overlap list.
@Daniel Rossberg please give feedback like the feasibility on the above mentioned idea or should I think differently.
Just a thought, Since I need a UI for it written in Tcl/Tk, I would not be able to call MGED's check command right? Instead I must have an executable version of this command.
Just a thought, Since I need a UI for it written in Tcl/Tk, I would not be able to call MGED's check command right? Instead I must have an executable version of this command.
Not necessarily. If you have a TCL shell which binds the mged commands you could use a mged check and present its output in a pleasant way.
but in rt there are a lot of global variables which makes it really easy for it to communicate
You need to get rid of the global variables and transform them to ordinary local ones and function parameters and return values respective.
I had started work on design of libanalyze, will post my progress in a few minutes.
parameters of the libanalyze:
* width & height
* cell width & cell height
* azimuth & elevation
* rpt_overlaps flag
* npsw
* mat flag
* objects : use the rt_gettrees in libged and pass the rtip as a parameter ? or pass the nobjs and objtab as parameters ?
* callback function pointer
* callback data pointer
Looks good.
The "azimuth & elevation" and "mat flag" are mutual exclusive. Maybe passing the model2view matrix would be the most flexible solution.
And, rtip sounds good.
Okay here is my progress: 15_05_progress.patch
It's a work in progress so there will some things which I kept like placeholder so that it compiles.
Looks good.
The "azimuth & elevation" and "mat flag" are mutual exclusive. Maybe passing the model2view matrix would be the most flexible solution.
And, rtip sounds good.
Okay I will add the passing of rtip as well.
The matflag is a bit challenging without globals, like adding all those function for rt_cmd_tab, the cm_* ones.
Also most of them take argc and argv as parameters so its best be done on the libged side only.
Okay there is a problem, the rt_cmdtab structure has the function pointers as :
int (*ct_func)(const int, const char **);
So they all need some global variables, if the function definition is fixed how will be able to pass the variables used inside them. :/
Don't pass a matflag, azimuth or elevation to analyze_overlaps() but the resulting matrix model2view (I think this is the interesting one). Do the computation of this matrix in the application (in libged ged_check, rt rtcheck, ...).
Don't pass a matflag, azimuth or elevation to analyze_overlaps() but the resulting matrix model2view (I think this is the interesting one). Do the computation of this matrix in the application (in libged ged_check, rt rtcheck, ...).
yep understood.
BTW, I wouldn't include the matflag feature in the libged ged_check(), it looks a little bit artificial, or?
Okay here is my progress: 15_05_progress.patch
It's a work in progress so there will some things which I kept like placeholder so that it compiles.
I'm only a little bit confused by the line
typedef void (*test)(void* callBackData);
I think you are over the test stage here and can give it a real name ;)
BTW, I wouldn't include the matflag feature in the libged ged_check(), it looks a little bit artificial, or?
yeah that's what I also thought :D.. for ged_check let's skip the -M option.
I think you are over the test stage here and can give it a real name ;)
Sure :) will make it real.
Otherwise it looks good, you are moving into the right direction.
Thank you for checking it !
How does -V option for aspect work? like we give two values then they are divided? We are essentially just mentioning the width/height ratio right? but I don't see it as mutually exclusive to height and width. What if the user gave two different values?
BTW, tomorrow (16th) I will be coding in the morning hours, I need to sleep early for my exam on 17th morning.
How does -V option for aspect work? like we give two values then they are divided? We are essentially just mentioning the width/height ratio right? but I don't see it as mutually exclusive to height and width. What if the user gave two different values?
See grid_setup() in src/rt/worker.c. And yes, this are only different ways to describe the width and height. It's confusing if they are used together.
Good luck for your exam :hand_with_index_and_middle_fingers_crossed:
See grid_setup() in src/rt/worker.c. And yes, this are only different ways to describe the width and height. It's confusing if they are used together.
Yeah i have to write that too because it is called by do_frame. So will have a look then.
Good luck for your exam :hand_with_index_and_middle_fingers_crossed:
Thank you :)
@Daniel Rossberg Please have a look and give your feedback, thanks :)
Here is my progress for today : 16_05_progress.patch
do_frame()
, do_run()
and grid_setup()
. struct resource resource[]
I couldn't avoid that for now :/ (Will try to avoid it)worker()
the bu_parallel()
had a fixed function pointer but gladly bu_parallel()
and worker()
had void *args
as parameter, so I was thinking to use struct to pass the variables required for worker inside it and pass this context with a void pointer. What do u say?worker()
but for parallel case I can pass it in bu_parallel()
's argJust a thought, Since I need a UI for it written in Tcl/Tk, I would not be able to call MGED's check command right? Instead I must have an executable version of this command.
@Saran Narayan it's FAR more important to get the logic that is currently in Tcl converted to C/C++ than it is to develop the UI...
@Sean I didn't get you, which tcl file? src/tclscripts/checker/check.tcl ?
Yes, doing what that command does as clean c/c++ instead of tcl, and by calling functions not programs
oh :D was not aware that it was to be done as well. That would require planning. I will first complete the libanalyze part and read about what the TCL file does. I never discussed it, I thought that I had to just port the shell script to C so it would be platform independent.
That means changes to project plan and timeline, I hope that is fine now.
Moving the code from the external program (src/rt) to a (core) library (src/libanalyze) is a prerequisite for being able to do the check by calling C/C++ functions (for example analyze_overlaps() from libanalyze) not programs (rtcheck).
@Saran Narayan
@Daniel Rossberg Please have a look and give your feedback, thanks :)
Here is my progress for today : 16_05_progress.patch
I see that you have improved the API around the analyze_overlaps_callback()
function.
What came into my mind there was that analyze_overlaps_callback()
has no callBackData
parameter. This is because you use directly the call back function for APP.a_overlap
. I think you should however use an adapter function between APP.a_overlap
and analyze_overlaps_callback()
which "translates" the ray-trace result to a more convenient form.
- I added
do_frame()
,do_run()
andgrid_setup()
.- I added model2view as a parameter, but with it came aspect and viewsize. (Will think of any ways to avoid the extras)
- To avoid more parameters passed to libanalyze I implemented part of grid_setup in libged and part of it in libanalyze.
This was to be expected. If one extracts a general usable library function from a program the program specific part of the logic stays at the program side.
- I also added a global variable
struct resource resource[]
I couldn't avoid that for now :/ (Will try to avoid it)
I'm not sure if this is possible. I had to look at bu_parallel() and its applications. But, you first ;)
- For
worker()
thebu_parallel()
had a fixed function pointer but gladlybu_parallel()
andworker()
hadvoid *args
as parameter, so I was thinking to use struct to pass the variables required for worker inside it and pass this context with a void pointer. What do u say?
For single cpu mode I would pass it directly inworker()
but for parallel case I can pass it inbu_parallel()
's arg
Yes the void*
parameter is for passing parameter(s) to bu_parallel()
's func
function pointer parameter. If there is more than one, and especially if they are of different type, a structure should be used.
I'm not sure if this is possible. I had to look at bu_parallel() and its applications. But, you first ;)
I'm don't think it currently is, at least not for anything that calls dirbuild. We need to unwind and eliminate each item stored in the resource struct, but each one presents a different challenge.
@Daniel Rossberg
Will post my progress in a few minutes
Here is it :
17_05_progress.patch
I still have to design the adapter for overlaps. Was having a doubt like if the call reaches the adapter it should have same definition as a_overlap which restricts me to pass the callback function pointer, I could pass the callback data using a_uptr. Maybe I am thinking it at the wrong direction.
I also tried the command with real life parameters, getting segfault :/ trying to debug it at the moment
found out the issue was at rt_gettrees on line 396 in check.c
how about
void adapter(struct application* app, const struct partition* part1, const struct bu_ptbl* ptbl, const struct partition* part2) { callback(part1, ptbl, part2, app->a_uptr); }
BTW, I hope you were successful at your exams.
found out the issue was at rt_gettrees on line 396 in check.c
humm I think I know why. It's because rt_gettrees expects rtip as a register but I am not passing it as a register
how about
void adapter(struct application* app, const struct partition* part1, const struct bu_ptbl* ptbl, const struct partition* part2) {
callback(part1, ptbl, part2, app->a_uptr);
}
not sure if I follow. Oh do u mean like writing the adapter function in ged_check and passing a function pointer to adapter instead of the add_overlaps right now. That should work
Oh do u mean like writing the adapter function in ged_check and passing a function pointer to adapter instead of the add_overlaps right now. That should work
No, the adapter is in libanalyze and will be called transparently (invisible for the user like ged_check()).
BTW, I hope you were successful at your exams.
well I do not get the results right away :D, Have to wait 2 months for the evaluation. Other than that it was fine..
Oh do u mean like writing the adapter function in ged_check and passing a function pointer to adapter instead of the add_overlaps right now. That should work
No, the adapter is in libanalyze and will be called transparently (invisible for the user like ged_check()).
then how will it know about the definition of callback ?
check.c is passing the function pointer to callback
to analyze_overlaps, so only analyze_overlaps knows about callback
right?
then how will it know about the definition of callback ?
Good point!
struct analyze_private_callback_data { analyze_overlaps_callback overlapHandler; void* overlapHandlerData; // the callBackData }; void adapter(struct application* app, const struct partition* part1, const struct bu_ptbl* ptbl, const struct partition* part2) { struct analyze_private_callback_data* callBack = (struct analyze_private_callback_data*) app->a_uptr; callBack->overlapHandler(part1, ptbl, part2, callBack->overlapHandlerData); }
yep this will work :D but it would make passing the function pointer as an explicit parameter to analyze_overlaps redundant as it is already being passed through the callback data
why did u add private in its name?
yep this will work :D but it would make passing the function pointer as an explicit parameter to analyze_overlaps redundant as it is already being passed through the callback data
That's why I called the structure analyze_private_~
, it's only visible (and created) in libanalyze.
oh I get it now. Thanks :) that helped
like
void analyze_overlaps() { struct analyze_private_callback_data callBack; callBack.overlapHandler = add_overlaps; callback.overlapHandlerData = callbackdata; APP.a_uptr = &callBack; }
yep exactly... took a few mins to click inside my head :D
gonna implement it now
found out the issue was at rt_gettrees on line 396 in check.c
humm I think I know why. It's because rt_gettrees expects rtip as a register but I am not passing it as a register
? Where is it mentioned as register
? And even register
is only a hint for the compiler.
The objtab
is more suspicious. With casting it you remove the possibility for the compiler to check it.
Why don't you use const char *objtab[];
?
And objtab = argv + (bu_optind+1);
? (Only an idea, untested.)
? Where is it mentioned as
register
? And evenregister
is only a hint for the compiler.
In rt/main.c it called def_tree(rtip)
looking at the definition in rt/do.c line 495, I saw it had rtip as register that is why I got that doubt.
Why don't you use
const char *objtab[];
?
Will see thanks
And bu_optind
is always 1 despite what happens in the while loop.
And
bu_optind
is always 1 despite what happens in the while loop.
I checked with a print statment giving -a10 -e10 it printed 3. So this is not true.
And
bu_optind
is always 1 despite what happens in the while loop.I checked with a print statment giving -a10 -e10 it printed 3. So this is not true.
OK, I see. Or, I couldn't see it because it's a global variable.
yep defined in bu/getopt.h
I am trying to figure out how to use objtab. The suggested method did not work because const char * objtab[] cannot be declared without a size.
I checked with a print statment giving -a10 -e10 it printed 3. So this is not true.
Hmm, if it's 3, in this case is the bu_optind + 1
okay? I mean, if there is no additional parameter, then objtab = argv + 2;
but shouldn't it be "+1"?
I added this statement for checking.
for (i=bu_optind;i<argc;i++){ bu_vls_printf(gedp->ged_result_str, " %s", argv[i]); } bu_vls_printf(gedp->ged_result_str, "\nargc: %d,bu_optind: %d",argc,bu_optind); return GED_OK;
Output was :
mged>check -a10 -e10 abc abcd abc abcd argc: 5, bu_optind:3
Hmm, if it's 3, in this case is the
bu_optind + 1
okay? I mean, if there is no additional parameter, thenobjtab = argv + 2;
but shouldn't it be "+1"?
you are right but this was for rtcheck so it expects the argv at bu_optind is always the titlefile and following that is the objects.
@Daniel Rossberg
I found the problem, it wasn't the objtab.
In original rt/main.c, rt_gettrees()
is called only after resource is init but I had moved resource to libanalyze so that wasn't possible. I added that initialize statements in check.c. That fixed it. But I cannot pass this resource instance to the libanalyze.
If I do rt_gettrees()
inside libanaylze then there is issue of do_ae()
running before rt_gettrees()
. If I wanted to move do_ae()
and rt_gettrees
to libanalyze that means more arguments(nobjs, objtab, azimuth, elevation) to be passed to libanalyze :/.
To test it out I moved the functions to libanalyze: 18_05_progress.patch
But still it won't work. Crashes after calling rt_shootray(&a)
. Trying to find out why.
Hmm, how about rtip = rt_new_rti(gedp->ged_wdbp->dbip);
in ged_check()
?
interesting, lemme try that out.
getting the crash after this in gdb:
655 RT_AP_CHECK(ap); (gdb) n Thread 8 "mged" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffda7f8700 (LWP 5821)] 0x00007fffe460caaf in Tk_FreeGC () from /home/sharan/brlcad/build/lib/libtk.so.8.5
I took this from voxelize.c It uses ray-trace as well.
yep I saw it in gqa.c as well
The rt_gettrees()
in analyze_overlaps()
maybe to much. It's supposed that they are already selected before calling analyze_overlaps()
.
And, in case of using the rt_i
with the data provided by mged the visible/edited objects should be already selected there.
yeah that is right
i saw this right now in gdb:
Thread 10 "mged" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffdbbfa700 (LWP 5861)] 0x00007ffff782a3ab in hit (ap=0x7fffdbbf9cf0, PartHeadp=0x7fffdbbf9600, segs=0x7fffdbbf9880) at /home/sharan/brlcad/src/libged/gqa.c:1258 1258 ((struct per_region_data *)pp->pt_regionp->reg_udata)->hits++;
not sure why the program would go to gqa's hit
That's a principle of analyze_overlaps()
: rt_i
already has the objects of interested selected.
When calling analyze_overlaps()
from libged it's the case with rtip = rt_new_rti(gedp->ged_wdbp->dbip);
. In case of calling it from rtcheck you have to call rt_gettree()
first.
That's a principle of
analyze_overlaps()
:rt_i
already has the objects of interested selected.
When callinganalyze_overlaps()
from libged it's the case withrtip = rt_new_rti(gedp->ged_wdbp->dbip);
. In case of calling it from rtcheck you have to callrt_gettree()
first.
yeah understood!
will revert back to 17_05 and try that
i saw this right now in gdb:
...
not sure why the program would go to gqa's hit
After calling the check
command? Beats me, I'll let you try to find the reason by yourself first. At the weekend I would be able to run it in a debugger then.
okay got it :)
@Daniel Rossberg
I added the line, rtip = rt_new_rti(gedp->ged_wdbp->dbip);
But I still have t o call rt_getree after that right? if I don't do_ae says i have no primitives active.
But if I add rt_getree the same error happens as before. Am I missing something?
@Daniel Rossberg
I added the line,rtip = rt_new_rti(gedp->ged_wdbp->dbip);
But I still have t o call rt_getree after that right? if I don't do_ae says i have no primitives active.
But if I add rt_getree the same error happens as before. Am I missing something?
No, you shouldn't have to call rt_getree()
, but have to select objects in mged first before you call check
.
yep I did, with draw command
If this won't work I need to have a deeper look at it. Maybe you can upload a patch file again if you are done for today.
rt_new_rti
doesn't do anything to rtip->nsolids
Maybe it isn't necessary. voxelize
works this way.
hmm but voxelize does call rt_gettree at line 190
Ups, indeed, must have overseen this...
that leaves us with one option to send resource[] to libanalyze. (is this possible?)
Isn't resource already in libanalyze? At least in your code?
BTW, I don't know if it's a good idea to give a variable the same name as its type.
Isn't resource already in libanalyze? At least in your quote?
To run rt_gettrees I need to prepare the resources before calling it.
BTW, I don't know if it's a good idea to give a variable the same name as its type.
Oh yeah I will correct that
@Daniel Rossberg
success! with 18_05_progress I changed the names of hit and miss functions to check_hit and check_miss. I think it was apparently calling gqa's hit and miss :O. The output printed the count of overlaps as expected :)
Now just need to figure out how to call rt_gettrees without moving everything to libanalyze.
i saw this right now in gdb: ...
not sure why the program would go to gqa's hit
This looks a bit to me like a resource was not initialized
awesome @Saran Narayan .. looks like great progress! don't be shy about suggesting changes to libanalyze, thinking about how it can/should be developed to do what you need it to do.
success! with 18_05_progress I changed the names of hit and miss functions to check_hit and check_miss. I think it was apparently calling gqa's hit and miss :O. The output printed the count of overlaps as expected :)
Now just need to figure out how to call rt_gettrees without moving everything to libanalyze.
hit and miss are very general function names which are very likely to be used somewhere else as well. You should work around this kind of issues in your code:
hit and miss are very general function names which are very likely to be used somewhere else as well. You should work around this kind of issues in your code:
* Don't use function and global variable names which could conflict with others. For example do_run, do_ae, etc. conflict with functions in src/rt. A solution could be a library, module, or file specific prefix like analyze_overlaps_ or analov_ or something like this.
* In addition, these functions should be marked for the compiler as local ones, only used in the actual source file. Then, they won't be exported and can't be used elsewhere. In BRL-CAD code this should be done with the HIDDEN define.
Yes! thanks for the heads up. Will do.
Hmm, with 18_05_progress.patch and the hit and miss function names changed I don't get overlaps:
mged> check -g10 -G10 truck.g g4 truck.g g4 argc: 5,bu_optind: 3 g4 Number of Overlaps: 0 Azimuth: 35.000000, elevation: 25.000000
Maybe you changed something else too?
oh.. I only remember adding rt_prep_parallel without which i got a few warnings. But without it too I had got output.
Anyway I will send the latest one with name changes we discussed.
creating patch.. I wonder why it takes so much time
Add changes to use rtip = rt_new_rti(gedp->ged_wdbp->dbip);
Now no need to mention file location. And objects count are without +1
Now it works :)
I'll look tomorrow in the resource vs. gettree issue then.
Okay thank you :)
I am thinking to now add code to populate the list.
Hmm, I couldn't find any issue: analyze.h , check_overlaps.c , check.c
Yeah in the 19/05 one, I have done rt_gettrees in libanalyze which you had said is not okay.
BTW, I was trying to get the overlap pairs to print in ged_check()
. I could't figure out why the output was all random.
pairs.png
Here is the patch : 20_05.patch
According to gdb:
add_overlaps (reg1=0x7fffd0000e30 "/g4/r25", reg2=0x7fffd0000d50 "/g4/r12", depth=0.39530180591737007, context=0x7fffffffc650)
It's getting the expected names.
BTW, I was trying to get the overlap pairs to print in
ged_check()
. I could't figure out why the output was all random.
When add_overlaps() was called there was a string at the address but when bu_vls_printf() is called the string is gone (overwritten with something else). You need to make copies of the strings which you own, e.g. with bu_strlcpy()
(it's a macro which calls bu_strlcpym()).
Don't forget to free the memory before leaving ged_check(), e.g. right after printing the names.
yep, I was trying that but with strcpy. Will use bu_strlcpy(). Thanks for the tip. :)
Okay got it working. Initially MGED got stuck, but after going through the definition of bu_strlcpym()
saw the usage of bu_semaphore_acquire(BU_SEM_SYSCALL)
and release, but I had acquired this semaphore in add_overlaps()
.
After removing them in add_overlaps
it was fine.
@Daniel Rossberg ,
I was planning to start with gqa today, so reading libged/gqa.c now. I needed feedback on how it should be implemented in libanalyze.
If I use an adapter like I did for rtcheck I could use the add_overlaps in libged/check.c. And make a new libanalyze c file that does the functions of gqa.
If I use an adapter like I did for rtcheck I could use the add_overlaps in libged/check.c. And make a new libanalyze c file that does the functions of gqa.
Which functions?
I remember @Sean saying to "add parallel capabilities of gqa into the libanalyze function", back in the application period.
a plan would probably be something like refactoring rtcheck into a libanalyze function, then refactoring rtcheck to use the new function, then adding the parallel capabilities of gqa into the libanalyze function, then refactoring gqa to use it, then adding your new command in libged that also uses that function (doing the work of both rtcheck and gqa)
I did not understand it back then. After going through the code of gqa seems like it shoots rays in 3 axises.
It's very complex and does many analysis functions, I only would be requiring the overlap part for this project.
So that I would get the overlap list just like what we did for rtcheck, but we also had the goal of making it free from the executable form.
But gqa is already in form of MGED command.
gqa puts a lot more functionality into the ray-trace. A single analyze_overlaps() isn't probable suited to be used there.
Another way would be to have a generalized frame work for shooting grids of rays and doing analysis with them.
And, analyze_overlaps() has already parallel capabilities.
I would recommend that you start to prepare a patch with your current work which can be applied to BRL-CAD's svn repository. You can use ttps://sourceforge.net/p/brlcad/patches for this.
gqa puts a lot more functionality into the ray-trace. A single analyze_overlaps() isn't probable suited to be used there.
Another way would be to have a generalized frame work for shooting grids of rays and doing analysis with them.
hmm, so how to proceed with the framework? any examples I can read :D?
And, analyze_overlaps() has already parallel capabilities.
Yeah, the parallel execution using threads right?
I would recommend that you start to prepare a patch with your current work which can be applied to BRL-CAD's svn repository. You can use https://sourceforge.net/p/brlcad/patches for this.
Okay will do it tonight! Anything I should keep in mind before submission? I had been cleaning up the code and adjusting indentation.
gqa puts a lot more functionality into the ray-trace. A single analyze_overlaps() isn't probable suited to be used there.
Another way would be to have a generalized frame work for shooting grids of rays and doing analysis with them.hmm, so how to proceed with the framework? any examples I can read :D?
This would mean to analyze both analyze_overlaps() and ged_gqa() for common functionalities and putting them into a common function.
And, analyze_overlaps() has already parallel capabilities.
Yeah, the parallel execution using threads right?
Right.
I would recommend that you start to prepare a patch with your current work which can be applied to BRL-CAD's svn repository. You can use https://sourceforge.net/p/brlcad/patches for this.
Okay will do it tonight! Anything I should keep in mind before submission? I had been cleaning up the code and adjusting indentation.
I saw several issues with your code but I'll judge after you have reviewed it by yourself. None of them was serious.
Okay got it, thanks. I will try to find the common functionalities between them and the review the code to submit the patch.
These are the similarities I found , they both do :
rt_init_resource
rt_gettree
rt_prep_parallel
RT_APPLICATION_INIT(&ap);
and many others like ap.a_overlap = overlap
From this, the similarity of the overlap function seems the most useful.
@Daniel Rossberg
Read you comments on the patch. About the object selection. Do you mean the active objects? or the rt_gettrees?
Exactly, something like
while (argc > 0) { if (rt_gettree(rtip,argv[0]) < 0) { bu_vls_printf(gedp->ged_result_str, "error: object '%s' does not exists, aborting\n", argv[1]); return GED_ERROR; } argc--; argv++; }
Okay thanks for the input, If rt_gettree can be run without the initialization of resource, then it would work!
That's why I had thought it was impossible with rt_gettrees.
Will do the requested changes and submit a new one :). Currently going through rt_default_multioverlap()
.
wow that worked :) now I have reverted to an old progress, hence I would be able to do do_ae and grid_setup in libged it self :D
@Daniel Rossberg please have a look at the v2 patch I had submitted yesterday
Don't worry ;)
BTW, why don't you use a_logoverlap?
hmm well since I was basically taking code from src/rt/. In viewcheck.c under view_init, logoverlap was set to rt_silent_logoverlap. That is why I did it that way
Do you plan to extend the mged check command with additional features?
I was planning to include the functionality of outputting plot file
hmm well since I was basically taking code from src/rt/. In viewcheck.c under view_init, logoverlap was set to rt_silent_logoverlap. That is why I did it that way
Do you know what rt_silent_logoverlap does? It's defined in src/librt/bool.c. Well, nothing you have to worry about.
Do you know what rt_silent_logoverlap does? It's defined in src/librt/bool.c. Well, nothing you have to worry about.
does nothing, just checks the magic number? for validity I think and returns.
where as rt_default_logoverlap prints lot of debug info to the user.
I was planning to include the functionality of outputting plot file
I mean, check
is a very general command. If you want to check for overlaps there only, check_overlaps or similar would be more suitable.
Do you know what rt_silent_logoverlap does? It's defined in src/librt/bool.c. Well, nothing you have to worry about.
does nothing, just checks the magic number? for validity I think and returns.
where as rt_default_logoverlap prints lot of debug info to the user.
For me it looks like for any reason a_logoverlap shouldn't stay empty. That's why there is the rt_silent_logoverlap(). If you have a better entry, like overlapsAdapter(), you can use it in a_logoverlap.
I mean,
check
is a very general command. If you want to check for overlaps there only, check_overlaps or similar would be more suitable.
Yeah true, initially I started writing that just to test the working of the libanalyze function.
I am still unclear about what I would be doing at the end.
Initially I had thought my task was to implement the functions the src/tclscripts/check.sh does because it was limited to linux and did lots of text processing.
After discussing with Sean, he said text processing must be removed. Hence the libanalyze function was the plan.
According to this I had planned that check command would take the objects as arguments and called libanalyze function to get the overlap lists in memory and did post processing similar to the shell script and hand it over to the check.tcl file.
But a few days ago I was told that I had to implement the check.tcl file in C aswell. So I believe now I can call libanalyze from that C file itself, so there would be no need of check command.
For me it looks like for any reason a_logoverlap shouldn't stay empty. That's why there is the rt_silent_logoverlap(). If you have a better entry, like overlapsAdapter(), you can use it in a_logoverlap.
Yeah if a_logoverlap is not set, there is seg-fault. Yep I could add something for a_logoverlap similar to overlapsAdapter, to do logging. Is this where the -r option helps? I am confused because -r option does not have an affect for rtcheck.
But a few days ago I was told that I had to implement the check.tcl file in C aswell. So I believe now I can call libanalyze from that C file itself, so there would be no need of check command.
At least not in src/tclscripts, but as a ged command you wrote a nice function. I don't want to do without it. When it has a more precise name ...
yeah well then check_overlaps sounds good for the name
I was thinking it could replace rtcheck command as well. Since it does almost everything what rtcheck does. But better because it's not calling exec_vp
Yeah if a_logoverlap is not set, there is seg-fault. Yep I could add something for a_logoverlap similar to overlapsAdapter, to do logging. Is this where the -r option helps? I am confused because -r option does not have an affect for rtcheck.
First, I would say a_logoverlap = overlapsAdapter
, or?
Looking at src/rt/opt.c you see that -r is the default. -R switches it off.
Looking at src/rt/opt.c you see that -r is the default. -R switches it off.
Yeah but look at the view_init in viewcheck.c. It does not consider any options.
I was thinking it could replace rtcheck command as well. Since it does almost everything what rtcheck does. But better because it's not calling exec_vp
Definitely! This is why you are doing this, moving the functionality to a central place at libanalyze where every module should use it.
It's the next step to let rtchek use analyze_overlaps().
Yeah but look at the view_init in viewcheck.c. It does not consider any options.
No, but overlap() and view_end() do.
Definitely! This is why you are doing this, moving the functionality to a central place at libanalyze where every module should use it.
It's the next step to let rtchek use analyze_overlaps().
Yeah sounds good! plus lesser use of globals :P
No, but overlap() and view_end() do.
hmm interesting, so overlap can do both logging and adding to list.
First, I would say
a_logoverlap = overlapsAdapter
, or?
How would this work? like passing the rpt_overlaps, then proceed in a similar fashion as overlap does? that wouldn't work because the signatures are different for logoverlap and multioverlap
hmm interesting, so overlap can do both logging and adding to list.
Adding to a list is a kind of logging, or vice versa: Logging is adding to a list.
ohk understood. I get it now.. multioverlap should actually used for resolving the overlaps
that's why it had code for deleting the left and right partitions
How would this work? like passing the rpt_overlaps, then proceed in a similar fashion as overlap does? that wouldn't work because the signatures are different for logoverlap and multioverlap
logoverlap hat const parameters and multioverlap not, that's all. Did I missed something?
that's why it had code for deleting the left and right partitions
Yes, this is something I'm still thinking about: Should analyze_overlaps() allow to change the geometry or not?
hmm yeah if it's allowed then "analyze"overlaps wouldn't be the right name.
If it's allowed tho then isn't like automatically doing the function of the overlap check tool
Checking a bank account doesn't mean to drain it ;)
However, the signatures of logoverlap and multioverlap are similar. It would be possible to switch if needed.
Haha yeah! It helps to visualize as well!
However, the signatures of logoverlap and multioverlap are similar. It would be possible to switch if needed.
yeah fine. So at the end an option to log or resolve the overlaps?
But how can we switch between them as one has const and one does not.
What if we switch inside the overlapHandler, just like it happens in viewcheck.c.
Pass overlapsHandler as multioverlap only. We pass the option as a int variable using the APP.a_user and decide.
But how can we switch between them as one has const and one does not.
You don't need to switch in analyze_overlaps(). You need only to decide if analyze_overlaps_callback can change the geometry or not. You can do logging with a_multioverlap as well, if you simply memorize that it was called and change nothing else. Bot, you remember the bank account? It would be more than just checking.
You don't need to switch in analyze_overlaps(). You need only to decide if analyze_overlaps_callback can change the geometry or not.
Did you mean like decide whether to modify based on the return values from analyze_overlaps_callback ?
You can do logging with a_multioverlap as well, if you simply memorize that it was called and change nothing else. Bot, you remember the bank account? It would be more than just checking.
Not sure if I understand what you mean here by memorizing and the reference to bank account.
You don't need to switch in analyze_overlaps(). You need only to decide if analyze_overlaps_callback can change the geometry or not.
Did you mean like decide whether to modify based on the return values from analyze_overlaps_callback ?
The parameters of a_multioverlap and a_logoverlap are essentially the same. The difference is in the const specifier. This means, algorithms written for a_logoverlap work with a_multioverlap as well. The reverse isn't true. If an algorithm requires a partition or bu_ptbl parameter to be modifiable it won't work with a_logoverlap.
I.e., if you want to be flexible use a_multioverlap.
You can do logging with a_multioverlap as well, if you simply memorize that it was called and change nothing else. But, you remember the bank account? It would be more than just checking.
Not sure if I understand what you mean here by memorizing and the reference to bank account.
Suppose you want to be flexible and use a_multioverlap. Then, you should make sure that a check command can't modify the database. I.e., analyze_overlaps() (the implementation of the general algorithm in libanalyze) could be used to modify a BRL-CAD database, but ged_check_overlaps() doesn't use this. It does logging only (i.e. writing something on the screen).
Suppose you want to be flexible and use a_multioverlap. Then, you should make sure that a check command can't modify the database. I.e., analyze_overlaps() (the implementation of the general algorithm in libanalyze) could be used to modify a BRL-CAD database, but ged_check_overlaps() doesn't use this. It does logging only (i.e. writing something on the screen).
Yup I got it. So as I was saying how to decide whether to modify or not? using a flag?
Yup I got it. So as I was saying how to decide whether to modify or not? using a flag?
No, you don't need a flag. Just make sure that ged_check_overlaps() implemetation of analyze_overlaps_callback (i.e. add_overlaps()) doesn' change a partition or bu_ptbl parameter.
No, you don't need a flag. Just make sure that ged_check_overlaps() implemetation of analyze_overlaps_callback (i.e. add_overlaps()) doesn' change a partition or bu_ptbl parameter.
Okay got it. Then next question comes to my mind is the adapter. It would be just to call the analyze_overlaps_callback with context passed to it right?
And are we restricting the user by providing a different signature for callback like one only capable of logging and one capable of modifying also
or just pass them without const and let the user decide..
In principle the adapter just hands over the parameters to the call-back function.
I had wished that the adapter preprocesses them as you are doing now, but I'm afraid this would't make things easier.
or just pass them without const and let the user decide..
Just pass them without const and let the user decide.
Cool! Thanks for clearing it up!
So that is for v3 patch. Anything else?
Cool! Thanks for clearing it up!
I had to think over this by myself as well.
So that is for v3 patch. Anything else?
Who knows? ;) But - yes, this should be all for patch 3.
@Daniel Rossberg
hmm does bu_ptbl_reset(regiontable)
count as modifying? because if I don't do that it wouldn't work gives seg-fault.
here is the work : check_overlaps_v3.patch
from include/rt/overlap.h it says at line 74:
a_logoverlap() function is intended for logging only, and a_multioverlap() is intended for resolving the overlap, only.
So according to me a_multioverlap
can't return without modifying.
Oh and I changed the commands name to check_overlaps but I kept the filename same, was thinking like if anyone adds some functions like check_* it could go inside check.c
Without bu_ptbl_reset(regiontable)
I was able to run the command for single process. It crashes when npsw is greater than 1.
But when in single process, I get bunch (counted and they were equal to number of overlaps) of errors saying that a_multioverlap was not able to resolve overlap. (check screenshot attached) Screenshot
Well, yes, it looks like I had a wrong understanding regarding what a_multioverlap does. I seems to resolve the overlap for the current ray-trace, without changing the underlying geometry. It is expected to tell the ray-trace algorithm what it shall do with the overlap. If it doesn't, a complaining message will be emitted.
I'ts true that a_multioverlap and a_logoverlap are getting the same arguments (see src/librt/bool.c lines 1704 and 1705), but there default implementations are very different. Whereas a_logoverlap can stay empty (see rt_silent_logoverlap() for an example), a_multioverlap needs a body which resolves the overlap somehow (like in rt_default_multioverlap()).
Because we aren't interested in how the overlaps are resolved in this ray-trace (a_hit and a_miss have no function), it's probable wise to leave a_multioverlap NULL and use this way rt_default_multioverlap(). I.e., set APP.a_logoverlap = overlapsAdapter
.
Oh and I changed the commands name to check_overlaps but I kept the filename same, was thinking like if anyone adds some functions like check_* it could go inside check.c
Use check_overlaps.c as file name similar to the bot_~.c stuff.
okay got it, just what I suspected.
So now should I revert back to v2 and use the overlapHandler to do pre processing or let the user decide like in v3?
Use check_overlaps.c as file name similar to the bot_~.c stuff.
Okay understood :)
And, I would need the patch on sourceforge.
okay got it, just what I suspected.
So now should I revert back to v2 and use the overlapHandler to do pre processing or let the user decide like in v3?
Leave like it is now (v3). You can improve the interface if it's advisable later on.
okay, will submit updated v3 on sourceforge
Working as expected now :)
Posted on sourceforge
OK, I've seen it.
hmm thinking what to start next.. refactor rtcheck or think about gqa?
refactor rtcheck
okay. So the current src/libged/rtcheck.c is like a wrapper to run the executable rtcheck application.
Now all that we have a libanalyze function we don’t need a wrapper I think
So I am thinking what really differentiates rtcheck.c from check_overlaps.c
we can call simply rtcheck it would then append the list of active primitives and run
there is option for outputting plot file
there is option for outputting plot file
I should be possible to write the file with the callback function or afterwards.
after checking viewcheck.c,
pdv_3space(outfp, rtip->rti_pmin, rtip->rti_pmax);
this happens in view_2init.
Will try to do this in libged before calling libanalyze
but there are some outfp related things going on in do_frame as well.
well in do_frame it only opens the file. So it can be done in libged as well
so, should I try it with check_overlaps or make a copy and name it rtcheck and add it there :D because of the similarity.
so, should I try it with check_overlaps or make a copy and name it rtcheck and add it there :D because of the similarity.
Seriously? Use analyze_overlaps()!
no you misunderstood.. that is what I meant only
wouldn't the libged part be the same for both? check_overlaps.c and rtcheck.c
Similar, but I would't say the same. rtcheck has the file output and the matrix commands.
right
matrix commands... that would require a lot of rt/do.c functions in libged.. and they all have fixed signatures according to rt_cmdtab
check include/rt/cmd.h on line 37
all the cm_* functions have the same signatures (const int argc, const char **argv)
and they all set variables but they are all in global scope.. how can I pass the variables to these function with fixed signatures.
Start with what you have in src/rt. If they require global variables there, use them, at least for the first version.
okay sounds good.
@Daniel Rossberg
I have added output as plot file and summary that is printed at the end for rtcheck. 26.05.progress.patch.
I didn't touch the existing rtcheck.c because I use that to test and compare behaviour. Will make it rtcheck during the final submission.
I was wondering how to give stdin in MGED, so that I can start work on matflag.
The only way to give the stdin I think is by making an executable version. So I removed all the ged code and renamed the ged to main and compiled an executable.
The rtcheck code belongs to src/rt, not src/libged. What they have in common is the libanalyze routine.
The mged/archer equivalent of rtcheck is check_overlaps. It doesn't need to write a file or execute matrix commands. (Rather, it should be enabled to use the current settings/view in mged and archer, but this is something we can think about later on.)
OK, you explained why you didn't want to overwrite the original code, but why did you implemented it in libged?
OK, you explained why you didn't want to overwrite the original code, but why did you implemented it in libged?
Yeah my bad. I did move it to src/rt now. :grimacing:
It isn't unusual to have multiple instances of a BRL-CAD repository on a machine, exactly because of this reason. E.g., I have 3 check-outs here. One of them is called vanilla. To avoid the extra network traffic you can simple make copies from your check-out and modify them independently.
Have you read my comment to your patch 488?
And, don't forget to write the documentation for the new check_overlaps command.
It isn't unusual to have multiple instances of a BRL-CAD repository on a machine, exactly because of this reason. E.g., I have 3 check-outs here. One of them is called vanilla. To avoid the extra network traffic you can simple make copies from your check-out and modify them independently.
yeah I have a fresh copy as zip file stashed away in downloads. When I require a clean repo I just extract it and check it out to latest version. :)
Have you read my comment to your patch 488?
Yeah I did read it. The code changes will do, but I tried the command in archer and it didn't even detect that there was a command check_overlaps.
And, don't forget to write the documentation for the new check_overlaps command.
yeah got it, will do.
You mentioned to "correct them with an own patch" which I did not understand. Should I comment the new patch or create a new ticket?
yeah I have a fresh copy as zip file stashed away in downloads. When I require a clean repo I just extract it and check it out to latest version. :)
This means that you have to take care of your disk memory?
Yeah I did read it. The code changes will do, but I tried the command in archer and it didn't even detect that there was a command check_overlaps.
<irony>Really?</irony> ;) It looks like you have something to do there.
You mentioned to "correct them with an own patch" which I did not understand. Should I comment the new patch or create a new ticket?
Create a new ticket.
This means that you have to take care of your disk memory?
yeah, your idea of vanilla seems better though, that way the checking out everytime I want a new repo is not there.
<irony>Really?</irony> ;) It looks like you have something to do there.
Hmmm, I will find it :D was just excited to start rtcheck first.
Create a new ticket.
Okay thanks for clearing it up!
When I say "3 check-outs" I mean I've 3 build directories too. This way I can easily switch between the versions. And, you could install them in different directories too. However, a build directory has a size of over 2 GByte.
oh yeah got it :). yeah 2.8 GB each..
BTW when I moved to srt/rt, I replaced all the bu_vls_printf(gedp->result_str,"") statements by fprintf(stdout,"") ones. That is fine right?
Using the bu_ functions is the recommended way. It's true that in src/rt usually fprintf() is used, but e.g. reshoot.c uses bu_vls_printf() as well. Therefore, it isn't forbidden to use fprintf() but it isn't necessary to replace bu_vls_printf() there.
It also depends on what you want to do with your code: Immediately write to the output or compose the whole string first.
okay understood. This just came to my mind, the script generated by saveview saves a logfile, it does this by redirecting stderr2> logfile.txt
.
So I was thinking to switch to stderr, would bu_vls_printf contents come inside stderr?
but stderr doesn't seem to be the right place for normal outputs
<irony>Really?</irony> ;) It looks like you have something to do there.
Figured it out :)
Screenshot-from-2018-05-26-23-34-35.png
though the 'g's appear to be cut off from the bottom..
submitted https://sourceforge.net/p/brlcad/patches/491/
bu_vls_printf() normally doesn't send anything to stdout or stderr, only if something bad happens like an unknown format specifier.
though the 'g's appear to be cut off from the bottom..
That's probable an issue of Archer'd terminal widget.
bu_vls_printf() normally doesn't send anything to stdout or stderr, only if something bad happens like an unknown format specifier.
Okay, I will test and figure out what to use accordingly
though the 'g's appear to be cut off from the bottom..
Looks OK on my machine: pasted image
hmm.. not sure why it is like that on mine.
edit: I think it could be because of the different distros we use and the fonts these distros use.
BTW I started to add that rt_cmd_tab things, it is sad that everything has to be made global for it work. Even parsing of the arguments must be done in function, because of cm_opt.
are all the cm_~ functions relevant for rtcheck?
are all the cm_~ functions relevant for rtcheck?
Probable not, but for the other src/rt programs.
Global variables in an executable aren't as bad as in a library. If an executable will be started multiple times the data of their instances are protected by running in different processes (each process has its own memory space).
Library functions could be called in parallel in different threads of the same process which can lead to problems with global variables (the threads of a process run all in the same memory space).
Global variables in an executable aren't as bad as in a library. If an executable will be started multiple times the data of their instances are protected by running in different processes (each process has its own memory space).
Library functions could be called in parallel in different threads of the same process which can lead to problems with global variables (the threads of a process run all in the same memory space).
Thanks for the explanation! :)
and saw your comment on the patch, working on it.
hmm so for pp - partp and hp - InputHdp, sounds good?
I couldn't find anything for pp, so partp is my guess :grimacing:
What do pp and hp stand for? The last "p" is probable for "partition", but the first letters?
h is for head
And p?
PartHead ?
This would be "ph", but you have "pp" and "hp". The second one could be *headPartition", but the first one?
meh.. It should be partPartition
Looking at src/librt/bool.c it looks like "pp" stands for "partition pointer" and "hp" for "head pointer". This isn't still very helpfull. Maybe rt_default_multioverlap() can explain this.
All-in-all it looks like that hp is useless.
And I've just realized that you included application in analyze_overlaps_callback. You shouldn't do this. This callback function has its own context.
And I've just realized that you included application in analyze_overlaps_callback. You shouldn't do this. This callback function has its own context.
yeah that is right :D, I kept ap unused there. Will correct that!
hmm, I cannot remove ap. Because it is need for plot file in rtcheck. But hp seems useless!
register struct xray *rp = &ap->a_ray; vect_t ihit; vect_t ohit; VJOIN1(ihit, rp->r_pt, ihitp->hit_dist, rp->r_dir); VJOIN1(ohit, rp->r_pt, ohitp->hit_dist, rp->r_dir); pdv_3line(outfp, ihit, ohit);
Looks like analyze_overlaps_callback needs a struct xray* parameter.
correction: const struct xray*
yeah true :)
I am giving rp the name rayPointer in analyze.h
ray or rayp is enough, or?
I think matflag is working for these commands, need to test the others
screenshot.png
woops no, I gave -a and -e so it deactivated matflag
okay removed and it still works :D
Here is the progress so far, @Daniel Rossberg
rtck.c
Only out of curiosity: Wasn't it possible to use functions from src/rt there, e.g. to do the command line parsing? ("No" is a valid answer ;)
there are a few changes like my do_frame is different and def_tree is different so anything that uses these functions need to be custom.
cmd_end, cm_multiview are different because of do_frame and def_tree
cm_tree is custom because I can only use rt_gettree not rt_gettrees
cm_start is custom because frame number is non functional for rtcheck I believe, my cm_start just returns without doing anything
cm_vsize, cm_eyept, cm_lookat_pt, cm_orientation can be reused because they are simple
some of them need APP.a_rt_i as global variable but I am not dealing with APP in rt, analyze_overlaps API uses it. But I have rtip, so they all can be changed to use rtip hence they need to be custom
OK.
Therefore you are making progress. Do you already have a schedule for your next patches?
BTW I was not able to test the old_way of processing stdin, i gave a text file containing 20 numbers, 1 for viewsize, 3 for eyept and 16 for Viewrotscale. I checked with rtcheck and my rtcheck both crashed with a bomb log
OK.
Therefore you are making progress. Do you already have a schedule for your next patches?
Nope, not yet planned.
Did you run it in a debugger? It should stop when the exception happens.
not really, since it crashed with both I thought it could be issue with my input.
Interpreting command stream in old format bn_mat_inv: singular matrix bn_mat_inv: singular matrix Saving stack trace to rtck-7930-bomb.log
this was the input :see_no_evil:
I am sure that viewrotscale is wrong
Shouldn't the input be commands, like the ones on your last screen shot?
that I believe is the new way.. "command driven"
if (fscanf(fp, CPP_SCAN(NUMBER_LEN), number) != 1) return -1; viewsize = atof(number); if (fscanf(fp, CPP_SCAN(NUMBER_LEN), number) != 1) return -1; eye_model[X] = atof(number); if (fscanf(fp, CPP_SCAN(NUMBER_LEN), number) != 1) return -1; eye_model[Y] = atof(number); if (fscanf(fp, CPP_SCAN(NUMBER_LEN), number) != 1) return -1; eye_model[Z] = atof(number); for (i = 0; i < 16; i++) { if (fscanf(fp, CPP_SCAN(NUMBER_LEN), number) != 1) return -1; Viewrotscale[i] = atof(number); }
this is the snippet of old way
BTW, the matrix in your example is really singular. Try
1 0 0 0 0 1 0 0 0 0 1 0 0 0 0 1
okay thanks
works now :D
gonna test everything else now. Will see if libged/rtcheck works
or are we replacing it? with check_overlaps?
if so then archer needs some changes too, like there is a button in archer to run rtcheck from the menu bar
and also the objects from view
via libged/rtcheck wrapper there is no need to mention the objects :)
Screenshot-from-2018-05-28-22-21-12.png
check_overlaps shall replace rtcheck. We want to get rid of calling another executable there. And the feature of to not need to mention the objects is something I would like to see in check_overlaps too.
And, the adaption/improvement of the GUI is also part of your project.
And, I would put back the gqa stuff. gqa does its own thing, and everything in one run.
check_overlaps shall replace rtcheck. We want to get rid of calling another executable there. And the feature of to not need to mention the objects is something I would like to see in check_overlaps too.
Hmm, will work on it and get it working!
And, the adaption/improvement of the GUI is also part of your project.
okay understood, once check_overlaps is ready will adapt it.
And, I would put back the gqa stuff. gqa does its own thing, and everything in one run.
Yeah but I need the output of these tools rtcheck and gqa in memory and not use text processing for the overlap checker tool I am gonna make
And, I would put back the gqa stuff. gqa does its own thing, and everything in one run.
Yeah but I need the output of these tools rtcheck and gqa in memory and not use text processing for the overlap checker tool I am gonna make
? You don't need the output of rtcheck, you have analyze_overlaps() and ged_check_overlaps() now. And, don't look at gqa, at least for the moment.
In that case it is fine. Yeah analyze_overlaps and check_overlaps can do the job
the checker.sh uses gqa for 2nd opinion, it can work with just the output of check_overlaps
oh there is also one more thing missing in check_overlaps the display of overlaps in yellow color
oh there is also one more thing missing in check_overlaps the display of overlaps in yellow color
This is something what has to happen in the callback function.
okay, I need to find the code that does it for rtcheck
looks like _ged_cvt_vlblock_to_solids()
does it on line 121 in libged/rtcheck.c
humm I don't get it. There is usage of vlblock which is new to me
I'm in doubt. Can it be that _ged_cvt_vlblock_to_solids() codes only "OVERLAPS" for printing on the screen and rt_process_uplot_value() does the real work?
yeah that would be right, _ged_cvt_vlblock_to_solids() gives it the name OVERLAPSffff00 and rt_process_uplot_value() does the real work of plotting it
#FFFF00 is for yellow color
something to do with plot file, let me make it display in txt for readability
plot file for rtcheck -g10 -G10 -a10 -e102 truck.g g4
And the feature of to not need to mention the objects is something I would like to see in check_overlaps too.
Done! :) @Daniel Rossberg
Screenshot-from-2018-05-29-01-10-51.png
Please have a look if it's proper, only the changes I did to check_overlaps.c :
visibleobjs.patch
Umm, I think I got plotting working, but again not sure if followed proper procedure to do that.
Screenshot-from-2018-05-29-21-32-12.png
as you see from the image I am using temp file.
Please have a look if it's proper, only the changes I did to check_overlaps.c :
visibleobjs.patch
oh I also found a bug with this just now, if I draw g4
. Then run check_overlaps g4
.
Then it runs g4 twice :/
Today's work it also has the yesterday's work of visibleobjs.patch
plot_overlaps.patch
Please have a look if it's proper, only the changes I did to check_overlaps.c :
visibleobjs.patch
How about either using the given objects if there are given or use the ones from ged_build_tops() if there are none?
that is a good idea!
will do it now.
The plotting shouldn't need a temp file. In principle it should possible to build the plot stream in the callback function and draw it afterwords (or meanwhile?)
It should be a kind of vlist: struct bu_list *vhead
, RT_ADD_VLIST()
, bn_vlist_2string()
to throw some keywords.
The plotting shouldn't need a temp file. In principle it should possible to build the plot stream in the callback function and draw it afterwords (or meanwhile?)
yeah that is what I was hoping to do but all the functions need a FILE*.
It should be a kind of vlist:
struct bu_list *vhead
,RT_ADD_VLIST()
,bn_vlist_2string()
to throw some keywords.
Didn't understand this..
The vlist is a list of vectors considered for displaying on the screen. The primitives have a rt_~_plot() function which generates such a list. These vector lists are used to generate the wireframe view in mged and Archer. The words I wrote are the list type and some functions/macro to build up such a list. I'm pretty sure that this is also how the highlighting of the overlaps works.
thanks, I think I am understanding it now. Like the function used rt_process_uplot_value
does essentially gets the 6 values from the plot file and uses BN_ADD_VLIST
to add these to vbp->free_vlist_hd
.
Look at rt_process_uplot_value() in src/librt/vlist.c. There you can see that the V x1 y1 z1x2 y2 z2
are translated to two BN_ADD_VLIST()
commands.
I think I could merge pdv_3line
and rt_process_uplot_value
into a custom function that does not use FILE *
Look at rt_process_uplot_value() in src/librt/vlist.c. There you can see that the
V x1 y1 z1x2 y2 z2
are translated to twoBN_ADD_VLIST()
commands.
Yeah exactly I remember the 6 values from the plot file I sent yesterday. First 3 for BN_VLIST_LINE_MOVE
and next 3 for BN_VLIST_LINE_DRAW
Give the callbackData a handle to this list (or a handle to the ged_check_ovl which contains this structure) and build it up in the callbacks.
by handle you mean the struct bu_list *vhead
returned from running these:
struct bn_vlblock *vbp = rt_vlblock_init(); struct bu_list *vhead = bn_vlblock_find(vbp, 0xFF, 0xFF, 0x00);
Yes. You are initializing chk_ovl this way. Maybe it's a good idea to keep this variable, but make it part of overlapData.
Then do what you are currently doing in plotoverlaps()->rt_process_uplot_value() in overlapHandler()->log_overlaps() now.
Okay got it :). And display the overlaps as overlay with _ged_cvt_vlblock_to_solids
at the end
Exactly.
Okay cool. There was one more bug I faced. Like once I ran check_overlaps and got the overlay if I run it again it would consider the OVERLAPSffff00 as a visible object and tries to rt_gettree it and crash.
Clean up the display first?
Sure thing!
Umm that object issues is solved now :)
And passed the structure to logoverlaps and tested with a simple print statement with gedp. So that is working as well. Gonna cleanup the display now.
Success! :) Thanks @Daniel Rossberg for such wonderful suggestions.
now it is just 3 lines to build up the list and display is 2 lines :D
Screenshot-from-2018-05-30-00-14-21.png
Okay cool. There was one more bug I faced. Like once I ran check_overlaps and got the overlay if I run it again it would consider the OVERLAPSffff00 as a visible object and tries to rt_gettree it and crash.
I don't understand why it thinks it a visible object. But I kinda hard-coded it for now :grimacing: to erase overlays just before buildingtops :
if ((db_lookup(gedp->ged_wdbp->dbip, "OVERLAPSffff00", LOOKUP_QUIET)) != RT_DIR_NULL) { dl_erasePathFromDisplay(gedp->ged_gdp->gd_headDisplay, gedp->ged_wdbp->dbip, gedp->ged_free_vlist_callback, "OVERLAPSffff00", 0, gedp->freesolid); }
hopefully there is another way
Okay cool. There was one more bug I faced. Like once I ran check_overlaps and got the overlay if I run it again it would consider the OVERLAPSffff00 as a visible object and tries to rt_gettree it and crash.
I'm most interested in rt_gettree crashing .. it shouldn't. can you provide a backtrace?
@Sean
https://pastebin.com/gkz865p7
This is 2nd run of check_overlaps
from the logs rt_gettree (rtip=0x13a64c0,
node=0x132abc0 "g4")
this is alright.
Then later there is one more call rt_gettree (rtip=0x13a64c0, node=0x0)
this is causing the crash.
Thread 1 "mged" received signal SIGSEGV, Segmentation fault. 0x00007fffe83d5253 in db_follow_path_for_state (tsp=0x7fffffffbf70, total_path=0x7fffffffbf30, orig_str=0x0, noisy=1) at /home/sharan/brlcad/src/librt/db_tree.c:849
Gonna investigate it
humm. So I ran the original rtcheck
then ran a modded version check_overlaps
that prints number of tops and names then returns.
Screenshot-from-2018-05-30-13-12-10.png
okay I think I know how rtcheck
doesn't crash when running it for the second time but check_overlaps does now.
rtcheck
uses ged_count_tops
to allocate memory for the gedp->ged_gdp->gd_rt_cmd
to fit the visible tops.
But it only considers the count of objects returned from ged_build_tops
.
So I did that experiment on my check_overlaps turns out ged_build_tops
returned one less number than ged_count_tops
when there are overlays displayed.
Screenshot-from-2018-05-30-13-33-16.png
BTW before fixing that I had one thing in mind,
I can run check_overlaps g4
and get it to display the overlays without actually drawing g4, which displays overlays just floating in the air making no sense.
I can also do this draw g2
and then doing check_overlaps g4
which would display overlaps of g4 as overlays on g2, again making no sense.
There are a these options to solve this in my opinion :
1) if a user runs check_overlaps g4
and g4 is not drawn then it shouldn't display the overlaps
But displays overlays when g4 is drawn. This method would require checking of mentioned objects and visible objects.
2) if a user runs check_overlaps g4
and g4 is not drawn then it could automatically draw g4 for the user and display the overlays.
3) like rtcheck
command does not allow to explicitly mention objects (but executable rtcheck does allow it) with command like rtcheck g4
, I could make check_overlaps work strictly for only visible objects.
Fixed the issue with a quick fix for now quick_fix.c
Will do a clean fix when the above doubt is cleared :)
Okay cool. There was one more bug I faced. Like once I ran check_overlaps and got the overlay if I run it again it would consider the OVERLAPSffff00 as a visible object and tries to rt_gettree it and crash.
I don't understand why it thinks it a visible object. But I kinda hard-coded it for now :grimacing: to erase overlays just before buildingtops :
hopefully there is another way
The question which came into my mind is: Should you clean up the display before creating a new overlay?
How does rtcheck handle this?
BTW, rtcheckdraws nothing here. Do I need to do something extra to get the overlay?
Umm it cleans up using the function call : _ged_cvt_vlblock_to_solids(gedp, chk_overlay.vbp, "OVERLAPS", 0)
Which calls invent_solid
in src/libged/display_list.c : 1132 which zaps the overlaps if drawing again
BTW, rtcheck draws nothing here. Do I need to do something extra to get the overlay?
nothing extra, try to run with a lower g,G value, less number of overlaps aren't easy to see. something like -g5 -G5 would make it display good.
OK, thanks. With 1 I can see something.
Do you have a current patch which allow me to look for the reason of the crash?
OK, thanks. With 1 I can see something.
Do you have a current patch which allow me to look for the reason of the crash?
I mentioned the reason above, it is clear now.
I quote it here:
okay I think I know how
rtcheck
doesn't crash when running it for the second time but check_overlaps does now.
rtcheck
usesged_count_tops
to allocate memory for thegedp->ged_gdp->gd_rt_cmd
to fit the visible tops.
But it only considers the count of objects returned fromged_build_tops
.
So I did that experiment on my check_overlaps turns outged_build_tops
returned one less number thanged_count_tops
when there are overlays displayed.
here is what i did to fix it:
check_overlaps_fix.patch
... and I had to wait some time until the overlay appeared.
Just looked at the implementation of ged_count_tops() and ged_build_tops(). These functions iterate over the display list. The overlay has a RT_DIR_PHONY_ADDR flag, which means that it isn't (yet) in the database.
tops -a
will show the overlays as well.
... and I had to wait some time until the overlay appeared.
performance issues? when I try with -g1 -G1 it takes around 3mins for me with 100% CPU
Just looked at the implementation of ged_count_tops() and ged_build_tops(). These functions iterate over the display list. The overlay has a RT_DIR_PHONY_ADDR flag, which means that it isn't (yet) in the database.
Yeah RT_DIR_PHONY_ADDR is set by invent_solid.
tops -a
will show the overlays as well.
yup saw it.
so are you suggesting that I write a custom ged_count_tops that skips the RT_DIR_PHONY_ADDR
flag?
like in ged_build_tops
it continues when flag is detected
Is this necessary? I think, the solution you found is okey. Can it be improved?
I'm satisfied.
Is this necessary? I think, the solution you found is okey. Can it be improved?
Yeah but the improvement would be negligible, because right now tobjtab = (char **)bu_calloc(tnobjs, sizeof(char *), "alloc tobjtab")
is allocating memory for one extra object which is not used.
only in case of when there are overlay
according to this image https://brlcad.zulipchat.com/user_uploads/1549/SkQANOUq25cKjDKHKyIyCuVb/Screenshot-from-2018-05-30-13-33-16.png
that null was the space allocated for overlay which is not filled by build_tops
BTW before fixing that I had one thing in mind,
I can runcheck_overlaps g4
and get it to display the overlays without actually drawing g4, which displays overlays just floating in the air making no sense.
I can also do thisdraw g2
and then doingcheck_overlaps g4
which would display overlaps of g4 as overlays on g2, again making no sense.
There are a these options to solve this in my opinion :
1) if a user runscheck_overlaps g4
and g4 is not drawn then it shouldn't display the overlaps
But displays overlays when g4 is drawn. This method would require checking of mentioned objects and visible objects.
2) if a user runscheck_overlaps g4
and g4 is not drawn then it could automatically draw g4 for the user and display the overlays.
3) likertcheck
command does not allow to explicitly mention objects (but executable rtcheck does allow it) with command likertcheck g4
, I could make check_overlaps work strictly for only visible objects.
also what to do about this problem
BTW before fixing that I had one thing in mind,
I can runcheck_overlaps g4
and get it to display the overlays without actually drawing g4, which displays overlays just floating in the air making no sense.
I can also do thisdraw g2
and then doingcheck_overlaps g4
which would display overlaps of g4 as overlays on g2, again making no sense.
There are a these options to solve this in my opinion :
1) if a user runscheck_overlaps g4
and g4 is not drawn then it shouldn't display the overlaps
But displays overlays when g4 is drawn. This method would require checking of mentioned objects and visible objects.
2) if a user runscheck_overlaps g4
and g4 is not drawn then it could automatically draw g4 for the user and display the overlays.
3) likertcheck
command does not allow to explicitly mention objects (but executable rtcheck does allow it) with command likertcheck g4
, I could make check_overlaps work strictly for only visible objects.
I would say: If the user wants to examine the overlaps in a different context (like the overlaps of g4 in the context of g2), do it. If there is no context yet: That's OK too.
ok then so that is solved, I have prepared a few more question to ask today.
should I print this information ? for the new rtcheck program.
View: 35 azimuth, 25 elevation off of front view Orientation: 0.248097, 0.476591, 0.748097, 0.389435 Eye_pos: 10588.7, 4181.26, 4710.67 Size: 11224.1mm Grid: (21.9221, 21.9221) mm, (512, 512) pixels
It was useful to me to debug but for the end user?
_ged_wait_status
defined in /libged/rtcheck.c:79 is declared in header file : libged/ged_private.h: 349
is used in libged/nirt.c : 530. So when I remove rtcheck.c from libged was thinking to move _ged_wait_status to libged/nirt.c
what to do about the rtcheck related code from cmd_rt
wrapper in mged/rtif.c : 57? remove it ? as I am using plain wrapper for check_overlaps
should I print this information ? for the new rtcheck program.
View: 35 azimuth, 25 elevation off of front view Orientation: 0.248097, 0.476591, 0.748097, 0.389435 Eye_pos: 10588.7, 4181.26, 4710.67 Size: 11224.1mm Grid: (21.9221, 21.9221) mm, (512, 512) pixelsIt was useful to me to debug but for the end user?
rtcheck prints similar stuff, but is in general annoying, that's true. How about giving check_overlaps a flag to print such debug information?
_ged_wait_status
defined in /libged/rtcheck.c:79 is declared in header file : libged/ged_private.h: 349
is used in libged/nirt.c : 530. So when I remove rtcheck.c from libged was thinking to move _ged_wait_status to libged/nirt.c
Sounds good and reasonable.
rtcheck prints similar stuff, but is in general annoying, that's true. How about giving check_overlaps a flag to print such debug information?
sounds good, by flag you meant like an option right?
what to do about the rtcheck related code from
cmd_rt
wrapper in mged/rtif.c : 57? remove it ? as I am using plain wrapper forcheck_overlaps
Yes, remove it. Does Archer have similar issues?
Yes archer also has similar code but I think it is exclusive for rtcheck, so I will need to adapt them for check_overlaps
rtcheck prints similar stuff, but is in general annoying, that's true. How about giving check_overlaps a flag to print such debug information?
sounds good, by flag you meant like an option right?
Yes, like -d
for debug or -v
for verbose.
Now related to -M flag,
there is a command multiview
if mentioned in the stdin would make rtcheck to run for a array of az/el values.
But my rtcheck works with error messages like :
WARNING: rt_prep_parallel(/home/sharan/brlcad/build/bin/truck.g, 1) invoked a second time, ignored
edit: found the mistake I did for the crash for original rtcheck
humm there is also seg fault at the end, I will investigate.
At first glance it looks like the plot file is kept the same.
The original rtcheck adds a suffix of digit for each run, have to find how it does this.
found it, so framenumber really has significance for rtcheck too :D
if (outputfile != (char *)0) { if (framenumber <= 0) { snprintf(framename, 128, "%s", outputfile); } else { snprintf(framename, 128, "%s.%d", outputfile, framenumber); }
But my rtcheck works with error messages like :
WARNING: rt_prep_parallel(/home/sharan/brlcad/build/bin/truck.g, 1) invoked a second time, ignored
also found this, it because I run rt_prep_parallel(rtip, npsw)
without checking rtip->needprep
If there is something left you couldn't fix provide me with a patch file and a description of how to reproduce the error. I'll look at it tomorrow then.
Thank you. I will try to fix these bugs tonight. If not I would have the patch posted here.
Fixed all the bugs for now, still have to test cm_anim and cm_set options.
humm fixed one major flaw in the overlapHandler, it was not properly giving overlap pairs. It was giving the same name for reg1 and reg2.
So the outputs of old rtcheck and new rtcheck didn't match
now the plotting also is correct :)
Here is the patch of all changes I did :
31.05.progress.patch
I just discovered one issue with check_overlaps, it doesn't get the values from the view like az/el, instead if not mentioned it uses the default values but rtcheck gets these values from the screen and runs rtcheck.
Will try to fix that issue, I think I figured it out
done :)
added a flag that is enabled by default and gets disabled when the user mentions az/el explicitly.
according to that flag this is run:
if(getfromview){ _ged_rt_set_eye_model(gedp, eye_model); viewsize = gedp->ged_gvp->gv_size; quat_mat2quat(quat, gedp->ged_gvp->gv_rotation); quat_quat2mat(Viewrotscale, quat); } else { check_do_ae(...) }
the rest is handled by grid_setup.
for printing the az/el value from the view2model generated from grid_setup I had to use:
vect_t work, temp; VSET(work, 0, 0, 1); MAT3X3VEC(temp, view2model, work); bn_ae_vec(&azimuth, &elevation, temp); bu_vls_printf(gedp->ged_result_str, "\nView: %g azimuth, %g elevation off of front view\n", azimuth, elevation);
I moved rt_prep_parallel(rtip, npsw)
to libged, rt from libanalyze because without preping it first I was getting the first value in the plot file as W 0 0 0 0 0 0, these are the values set by pdv_3space(outfp, rtip->rti_pmin, rtip->rti_pmax)
.
rt_prep_parallel
sets the values for rtip->rti_pmin and rtip->rti_pmax. Since I am not passing outfp to libanalyze, I decided to do rt_prep_parallel
in libged, rt.
I moved
rt_prep_parallel(rtip, npsw)
to libged, rt from libanalyze because without preping it first I was getting the first value in the plot file as W 0 0 0 0 0 0, these are the values set bypdv_3space(outfp, rtip->rti_pmin, rtip->rti_pmax)
.
rt_prep_parallel
sets the values for rtip->rti_pmin and rtip->rti_pmax. Since I am not passing outfp to libanalyze, I decided to dort_prep_parallel
in libged, rt.
This sounds like it could cause problems if somebody doesn't call rt_prep_parallel() before analyze_overlaps(). It is a bad thing if there are non-obvious preconditions for a function call.
okay I have a solution in mind,
if (rtip->needprep) { rt_prep_parallel(rtip, npsw); }
This would be in libanalyze and if I want I can do rt_prep_parallel before as well without no harm
What exactly is the issue with calling rt_prep_parallel() in libanalyze? rti_pmin and rti_pmax are different then? Why?
umm for outputting plot file in rtcheck, the first row is W 0 0 0 0 0 0
, which is wrong,( though W doesn't have any significance )
That is caused by rti_pmin and rti_pmax not having any value initialized.
Their values are initialized by rt_prep_parallel.
once analyze_overlaps is called the only way back to libged is the overlapHandler which is called multiple times for per overlap, so that was not the right place to put the 'first' row of plot-file.
I would say the line pdv_3space(outfp, rtip->rti_pmin, rtip->rti_pmax);
in src/rt/rtcheck.c is wrong. Use the right values there.
Did not understand what you mean by right values.
According what you say, rtip->rti_pmin and rtip->rti_pmax are uninitialized when you call pdv_3space(). What are the right values? The bounding box of the geometry? Should the line be therefore pdv_3space(outfp, bounding_box_min, bounding_box_max);
?
oh the rtip->mdl_min
and rtip->mdl_max
values?
but there some kind of processing happening on rtip->mdl_min
and rtip->mdl_max
on line 377 src/librt/prep.c
Yes, this is true. If you want to get the min-max-box you have to call rt_prep() or rt_prep_parallel() first.
Looking at my own code I saw that I write if (m_rtip->needprep)
before calling the preparation function. Therefore this is probable what you should do too in analyze_overlaps().
And, only when you need the min-max-box you should call the prep function in advance, i.e. for example in rtcheck.c.
Okay cool, so I would remove that from libged/check_overlap.c but keep it in src/rt/rtcheck.c and in libanalyze will check if it needs prep and call rt_prep
The getting from view code I wrote today is fine right? which is not there in progress patch I sent.
Do you mean this one?
if(getfromview){ _ged_rt_set_eye_model(gedp, eye_model); viewsize = gedp->ged_gvp->gv_size; quat_mat2quat(quat, gedp->ged_gvp->gv_rotation); quat_quat2mat(Viewrotscale, quat); } else { check_do_ae(...) }
This looks OK.
yup. Okay :thumbs_up:
now what remains to test is cm_anim (rt/do.c:360) option and cm_set (rt/do.c:437) option for matflag,
I had tried cm_set with just the keyword 'set' but it didn't print anything for both rtcheck and old rtcheck. So I am not sure how it works
set should work as set width=10
for example. You can see the possible values in src/rt/do.c:410 ff. I don't know which of them (if any) are relevant for rtcheck.
anim is marked as experimental, probable not considered to be used with rtcheck. See e.g. http://brlcad.org/OLD/reports/tr-313/chapt3.html#The%20RT%20Matrix%20Operations%20for%20Animation
okay with set width=10 and set height=10 it is working
humm I had tried set width = 512
the space caused problems
The parser is implemented in src/libbu/parse.c. It is a very simple one.
yup got it :thumbs_up:
so I think rtcheck is ready. These were the patches I had in mind to submit:
umm I was getting some random outputs as overlays :
scrambled.png
and crash on a re-run of check_overlaps:
Thread 17 "mged" received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffdb168700 (LWP 11854)] 0x00007ffff77c8405 in log_overlaps (reg1=0x14ff1b0 "/g4/r80", reg2=0x13165d0 "/g4/r64", depth=3.4213709577525151, ihit=0x7fffdb167320, ohit=0x7fffdb167340, context=0x7fffffffc430) at /home/sharan/brlcad/src/libged/check_overlaps.c:68 68 BN_ADD_VLIST(callbackdata->vbp->free_vlist_hd, callbackdata->vhead, ihit, BN_VLIST_LINE_MOVE);
What I noticed was that the number of overlaps were not matching like the summary prints different count but my count variable says different. So I tried with -P 1
option and it worked fine without any strange behaviours.
Hence it was a issue of threads so I protected the critical section with semaphores in overlapHandler and it fixed it.
bu_semaphore_acquire(BU_SEM_SYSCALL); log_overlaps(reg1->reg_name, reg2->reg_name, depth, ihit, ohit, context); bu_semaphore_release(BU_SEM_SYSCALL);
found one more issue,
if I run check_overlaps g4
it would plot the overlays.
But if next I run check_overlaps
the check if (tnobjs <= 0 )
passes because ged_count_tops
considers the overlay as object and continues execution which is wrong and I get an error from rt_prep_parallel saying no primitives left to prep
so to fix this I added this check if (tnobjs <= 0 )
once again after ged_build_tops
but now the check if (tnobjs <= 0 )
fails and it aborts the execution because ged_build_tops
corrects the count.
one other way to solve it would be write the ged_count_tops
code inside check_overlaps.c to not count RT_DIR_PHONY_ADDR
objects
@Daniel Rossberg
I implemented the overlaps_list using bu_list. Because there was a comment (rt/viewcheck.c:341) about the need of doubly linked list to iterate the list backwards looking for duplicate entries.
rtcheck.c
It looks like you are on a good way. I'm looking forward to get the patches from you which I can apply to the trunk.
Thank you :)
Yep I will send the patches soon.. just testing everything to solve any bugs.
found one more issue,
if I runcheck_overlaps g4
it would plot the overlays.
But if next I runcheck_overlaps
the checkif (tnobjs <= 0 )
passes becauseged_count_tops
considers the overlay as object and continues execution which is wrong and I get an error from rt_prep_parallel saying no primitives left to prep
so to fix this I added this checkif (tnobjs <= 0 )
once again afterged_build_tops
but now the checkif (tnobjs <= 0 )
fails and it aborts the execution becauseged_build_tops
corrects the count.
one other way to solve it would be write theged_count_tops
code inside check_overlaps.c to not countRT_DIR_PHONY_ADDR
objects
what should I do about this? the 2nd time check was a quick fix
OK, this issue is still open. I'll look at it tomorrow. Can I reproduce it with your last patch?
I will send a full patch right now :thumbs_up:
oh and I saw these function calls in libged/rtcheck: line 283
GED_CHECK_DRAWABLE(gedp, GED_ERROR)
and GED_CHECK_VIEW(gedp, GED_ERROR)
Not sure what they do
they check _gedp->ged_gdp == GED_DRAWABLE_NULL
and _gedp->ged_gvp == GED_VIEW_NULL
but what do they signify?
Maybe the gedp hasn't always a view attached. I cannot test it at the moment, but you can start mged without graphics attached, for example.
yep that is it, I launched mged with -c and ran rtcheck it said A view does not exist.
You see, we learned something again :)
okay then once that issue is resolved I would submit the v2 for patch #491, gonna review rtcheck for submission in the meantime.
/* else copy all the objects in view if any */ if (nobjs <= 0) { nvobjs = ged_build_tops(gedp, objp, &tobjtab[tnobjs]); /* now, as we know the exact number of objects in the view, check again for > 0 */ if (nvobjs <= 0) { bu_vls_printf(gedp->ged_result_str,"no objects specified or in view, aborting\n"); bu_free(tobjtab, "free tobjtab"); tobjtab = NULL; return GED_ERROR; } }
What bothers me more is the if (!nobjs && getfromview) {
line. It makes that draw g4
+ check_overlaps
gives a different result as check_overlaps g4
, but I don't have a better idea. Omitting the !nobjs &&
part leads to not finding any overlap because of a degenerated view.
Probably it's best as it is.
/* else copy all the objects in view if any */ if (nobjs <= 0) { nvobjs = ged_build_tops(gedp, objp, &tobjtab[tnobjs]); /* now, as we know the exact number of objects in the view, check again for > 0 */ if (nvobjs <= 0) { bu_vls_printf(gedp->ged_result_str,"no objects specified or in view, aborting\n"); bu_free(tobjtab, "free tobjtab"); tobjtab = NULL; return GED_ERROR; } }
yeah this looks good, a comment so that it's not confusing
What bothers me more is the
if (!nobjs && getfromview) {
line. It makes thatdraw g4
+check_overlaps
gives a different result ascheck_overlaps g4
, but I don't have a better idea. Omitting the!nobjs &&
part leads to not finding any overlap because of a degenerated view.
it gives a different result because, when doing check_overlaps g4
it expects the view info from the user and if not provided uses the default values on the other hand draw g4
+ check_overlaps
takes the view info and checks for overlaps as it is shown on the screen
welp the recent changes broke multiview command, but I know how to fix it :)
I am only initializing overlaplist pointer olist in main and free it in printoverlaps fuction.
but multiview doesn't exit instead it loops in do_frame, so in 2nd run causes seg fault.
hence I am moving the initializing of overlaplist to do_frame therefore for every run it creates a new overlaplist. and frees the overlaplist after printing them
it gives a different result because, when doing
check_overlaps g4
it expects the view info from the user and if not provided uses the default values on the other handdraw g4
+check_overlaps
takes the view info and checks for overlaps as it is shown on the screen
Understood, but the idea was: If an information isn't provided by command line parameters, use the view to get them. The issue which came up with this is, that the view may not contain no useful data too. Therefore, it's probable OK as it is now.
I am only initializing overlaplist pointer olist in main and free it in printoverlaps fuction.
but multiview doesn't exit instead it loops in do_frame, so in 2nd run causes seg fault.
Then, why don't you free it in main()?
I am only initializing overlaplist pointer olist in main and free it in printoverlaps fuction.
but multiview doesn't exit instead it loops in do_frame, so in 2nd run causes seg fault.Then, why don't you free it in main()?
yeah that works but wouldn't I need a new overlaplist for every run with different az/el values else all the runs would have the data from previous run?
Expected : rtcheck_old_output.txt
What I get if free only in main: rtcheck_output.txt
check frame 1 and frame 2 the difference would be clear. rtcheckold gets 0 overlaps but my rtcheck since it is freeing only in the end gets the same overlaps as frame 0 because it appends to the previous result
OK, that's why you moved the allocation and free to rtcheck_do_frame(). Sounds reasonable.
yep :thumbs_up:
okay so I am gonna submit the patch for check_overlaps
will do a quick test on archer and see how it behaves with plotting now
Accepted. Some comments:
okay got it. Will do one more patch fixing that.
Made additional changes as well. Using const char for options and help string and a typo fix.
@Daniel Rossberg
I don't know how to fix the comment on line 421 rt/do.c its in the struct bu_structparse set_parse[]
, I have copied this struct in rtcheck.c too.
in the meantime I thought I will start working on the next patch, that is adaptation of GUI to use check_overlaps instead of rtcheck and get rid of libged/rtcheck.c
while doing that I hit a few questions like:
1) tclscripts/lib/Display.tcl and tclscripts/lib/Drawable.tcl there is definition of rtcheck but the file is deprecated, so I can just remove them right? without adding check_overlaps there.
2) in tclscripts/lib/Ged.tcl at line 3213 there is a method called pane_rtcheck. I am not sure what it does different than normal rtcheck which is also defined in the file.
@Daniel Rossberg
I don't know how to fix the comment on line 421 rt/do.c its in thestruct bu_structparse set_parse[]
, I have copied this struct in rtcheck.c too.
I see two solutions:
1. Leave it as it is. This may not be optimal, but you don't change the behavior this way. This is a valid course of action.
2. Introduce rtcheck_bot_minpieces etc. variables and use bu_byteoffset(rtcheck_bot_minpieces)
etc. in set_parse. Then set rtcheck_bot_minpieces = rt_bot_minpieces;
etc. before parsing the arguments and rt_bot_minpieces = rtcheck_bot_minpieces;
etc. afterward.
hmm, yeah 2nd option seems fine would try it out..
while doing that I hit a few questions like:
1) tclscripts/lib/Display.tcl and tclscripts/lib/Drawable.tcl there is definition of rtcheck but the file is deprecated, so I can just remove them right? without adding check_overlaps there.
Right.
2) in tclscripts/lib/Ged.tcl at line 3213 there is a method called pane_rtcheck. I am not sure what it does different than normal rtcheck which is also defined in the file.
You probable need the pane_~ method too. They look like a window specific version of the commands, and you handle with the view in ged_check_overlaps().
okay got it thanks!
but then normal rtcheck uses something like :
eval $mGed rtcheck $itk_component($itk_option(-pane)) $args
but my check_overlaps just uses :
eval $mGed check_overlaps $args
I tried the same like :
eval $mGed check_overlaps $itk_component($itk_option(-pane)) $args
But gave an error :
Error: error: object '.archer0.hpane.pane0.childsite.vpane.pane1.childsite.canvasF.mged.pane0.childsite.pw.pane1.childsite.view' does not exists, aborting.
and these in logs..
db_lookup(.archer0.hpane.pane0.childsite.vpane.pane1.childsite.canvasF.mged.pane0.childsite.pw.pane1.childsite.view) failed: .archer0.hpane.pane0.childsite.vpane.pane1.childsite.canvasF.mged.pane0.childsite.pw.pane1.childsite.view does not exist db_string_to_path() of '.archer0.hpane.pane0.childsite.vpane.pane1.childsite.canvasF.mged.pane0.childsite.pw.pane1.childsite.view' failed on '.archer0.hpane.pane0.childsite.vpane.pane1.childsite.canvasF.mged.pane0.childsite.pw.pane1.childsite.view' db_walk_tree: warning - .archer0.hpane.pane0.childsite.vpane.pane1.childsite.canvasF.mged.pane0.childsite.pw.pane1.childsite.view not found.
Maybe, you can find out how the other commands handle the pane information?
okay sure I will have a look at it :)
will post my progress for today
Screenshot-from-2018-06-04-23-38-34.png
I also noticed when I draw it won't show up on the screen, I tried the same with prebuild archer it worked fine. Is it a archer bug? or did I do something :/
Archer-7.27.0_011.png
Hmm, draw g4
works for me (compiled yesterday with your patch).
oh and you didn't do anything extra?
doing rt
shows it but it wont show as wireframe
But, didn't you posted Archer screen shots here with wireframe images on them?
nope I have not, all screenshots were from mged. Is compiling any different, I just do cmake ..
and then make -j4
Beats me, have you checked that you've installed all required libraries from https://brlcad.org/wiki/Compiling? Does the cmake run print any complains about missing libraries?
cmake logs
I don't see any complaints though there are a few things it says not found etc
but if its critical it should have aborted it :thinking_face:
all the dependencies on https://brlcad.org/wiki/Compiling are statisfied
I see, that you don't compile with OpenGL. This could lead to unexpected behavior.
Okay I will try with OpenGL. Let me see if I can figure out how to do that
yes! that was it. Now it is working great :)
Yeah, Archer (unlike MGED) requires OpenGL for display.
what's odd is that archer actually started with opengl compiled off
Odd indeed
humm after hours of figuring out how pane info is handled in tclscripts/archer/ I had almost given up :/
But @Daniel Rossberg your suggestion helped ; ) I tried to backtrace rtcheck on a vannila build and found out that it was actually handled in libtclcad/tclcadobj.c using to_view_func
.
changing {"check_overlaps", (char *)0, TO_UNLIMITED, to_pass_through_func, ged_check_overlaps}
to {"check_overlaps", (char *)0, TO_UNLIMITED, to_view_func, ged_check_overlaps}
solved it :)
I also ended up adding cmd_~ wrapper for check_overlaps instead of plainwrapper.
int cmd_check_overlaps(ClientData UNUSED(clientData), Tcl_Interp *interp, int argc, const char *argv[]) { int ret; Tcl_DString ds; CHECK_DBI_NULL; /* skip past _mged_ */ if (argv[0][0] == '_' && argv[0][1] == 'm' && bu_strncmp(argv[0], "_mged_", 6) == 0) argv[0] += 6; Tcl_DStringInit(&ds); ret = ged_check_overlaps(gedp, argc, (const char **)argv); Tcl_DStringAppend(&ds, bu_vls_addr(gedp->ged_result_str), -1); Tcl_DStringResult(interp, &ds); if (ret == GED_OK) return TCL_OK; return TCL_ERROR; }
It is quite similar to cmd_rt (just difference is ged_check_overlaps
in-place of ged_rt
), but I don't know how this will help xD. Like when does the skipping of _mged_ is required?
hey the adaptation helped MGED also. Now I can press tab after type ch and it will complete to check_overlaps :)
Today's progress..
5.06.progress.patch
Impressive :)
Regarding _mged_ I had to look at the code (at the earliest on Thursday). But, my first idea would be that this TCL script is considered to run on mged too. However, I'm not sure, had to look at the code first.
OK, you found this too :)
BTW i submitted the patch for rtcheck, in that I had deleted viewcheck.c but later I thought is deleting okay? should I have had put a deprecated comment and just remove it from cmakelists?
I have started to update the documentation now ..
humm I don't understand why I can't see the changes. make says it built but when I open the html page no change :/
okay I am seeing the changes I made in man1 and mann in MGED
humm I don't understand why I can't see the changes. make says it built but when I open the html page no change :/
Okay scratch that xD I was looking the wrong directory
So I did that experiment on my check_overlaps turns out
ged_build_tops
returned one less number thanged_count_tops
when there are overlays displayed.
I can see how this would be confusing, one building from the .g and the other counting what's displayed (not from the .g)
1) if a user runs
check_overlaps g4
and g4 is not drawn then it shouldn't display the overlaps
But displays overlays when g4 is drawn. This method would require checking of mentioned objects and visible objects.
On the surface, this seems reasonable behavior. I'd suggest also adding a flag to explicitly draw or not draw overlaps, something like "check overlaps -V g4" and "check overlaps -v g4" to turn off/on respectively.
3) like
rtcheck
command does not allow to explicitly mention objects (but executable rtcheck does allow it) with command likertcheck g4
, I could make check_overlaps work strictly for only visible objects.
Also reasonable.
It is quite similar to cmd_rt (just difference is
ged_check_overlaps
in-place ofged_rt
), but I don't know how this will help xD. Like when does the skipping of _mged_ is required?
You don't really need to worry about the _mged_ bit. But if you want to know, it's because all commands are registered twice with the Tcl interpreter, one with and without a _mged_ prefix. This is because users can override commands and certain places in the code, we need to make sure we're calling the original.
1) if a user runs
check_overlaps g4
and g4 is not drawn then it shouldn't display the overlaps
But displays overlays when g4 is drawn. This method would require checking of mentioned objects and visible objects.On the surface, this seems reasonable behavior. I'd suggest also adding a flag to explicitly draw or not draw overlaps, something like "check overlaps -V g4" and "check overlaps -v g4" to turn off/on respectively.
@Sean I thought about this for a while and discovered the following cases:
Implementation can be a bit complicated but not impossible.
And when thinking all this I found a flaw, what if the user says check_overlaps g4 g4
the current implementation of check_overlaps doesn't handle this exception.
3) like
rtcheck
command does not allow to explicitly mention objects (but executable rtcheck does allow it) with command likertcheck g4
, I could make check_overlaps work strictly for only visible objects.Also reasonable.
At the end this seems best with very straight forward implementation.
It is quite similar to cmd_rt (just difference is
ged_check_overlaps
in-place ofged_rt
), but I don't know how this will help xD. Like when does the skipping of _mged_ is required?You don't really need to worry about the _mged_ bit. But if you want to know, it's because all commands are registered twice with the Tcl interpreter, one with and without a _mged_ prefix. This is because users can override commands and certain places in the code, we need to make sure we're calling the original.
okay, I did find the code that adds the _mged_ prefix but was confused when I saw it being registered twice with Tcl_CreateCommand. Thanks for clearing the confusion :)
In general your task is a restructuring and refactoring one, so you certainly don’t have to figure out the best usage.. you can focus on the code and making it do what it currently does. Overlaps are just one of a whole suite of things that users want to check, and even overlaps aren’t done the best we could.
That’s to say you’re doing great :) keep up the good work!
In general your task is a restructuring and refactoring one, so you certainly don’t have to figure out the best usage.. you can focus on the code and making it do what it currently does. Overlaps are just one of a whole suite of things that users want to check, and even overlaps aren’t done the best we could.
Okay understood! So I will make check_overlaps behave like existing rtcheck command and always get objects from view.
And I fixed the flaw of duplicate objects given for the my version of rtcheck program, because the existing rtcheck handles it fine.
Here is what I did:
for (i=0; i<nobjs; i++) { if (add_to_objlist(objtab[i], &objlist) < 0) bu_log("'%s' object repeated, duplicates ignored\n",objtab[i]); }
HIDDEN int add_to_objlist(const char *object, struct object_list **objlistp) { struct object_list *op; struct object_list *new_op; BU_GET(new_op, struct object_list); for (BU_LIST_FOR(op, object_list, &((*objlistp)->l))) { if ((BU_STR_EQUAL(object, op->name))) { bu_free((char *) new_op, "object list"); return -1; } } new_op->name = object; BU_LIST_INSERT(&((*objlistp)->l), &(new_op->l)); return 0; }
did the same for the tree command that is used to input obj list from a file using matflag.
PS: don't mind indentation shown here, It's alright in the actual program ;)
That’s to say you’re doing great :) keep up the good work!
Thank you :)
now check_overlaps will take objects from view only. I also updated the related help data too.
humm I don't understand the relevance of doc/html/manuals/cadwidgets/ and also Database, Display, Drawable, QuadDisplay, MGED are deprecated. So as discussed before I am removing rtcheck related things but not adding check_overlaps there.
doc/tool_categories.txt is the list of programs or commands? it seems like list of programs. So not adding check_overlaps there.
@Daniel Rossberg If I submit a v2 for https://sourceforge.net/p/brlcad/patches/494/ should I do it with the changes I did today to check_overlaps?
In general your task is a restructuring and refactoring one, so you certainly don’t have to figure out the best usage.. you can focus on the code and making it do what it currently does. Overlaps are just one of a whole suite of things that users want to check, and even overlaps aren’t done the best we could.
This doesn't mean however that you can't realize own ideas and improve the behavior.
I don't understand what the issue with the drawing of overlaps of not displayed elements is. What's the problem there? I've rather a problem with drawing the overlaps on visible elements because they are hard to see. I've just triedcheck_overlaps g4
an a clear screen in Archer. This gave me a better view on the problem areas as I got before.
@Daniel Rossberg If I submit a v2 for https://sourceforge.net/p/brlcad/patches/494/ should I do it with the changes I did today to check_overlaps?
Put them back.
doc/tool_categories.txt is the list of programs or commands? it seems like list of programs. So not adding check_overlaps there.
Right.
I don't understand what the issue with the drawing of overlaps of not displayed elements is. What's the problem there? I've rather a problem with drawing the overlaps on visible elements because they are hard to see. I've just tried
check_overlaps g4
an a clear screen in Archer. This gave me a better view on the problem areas as I got before.
Agreed they are hard to see with objects shown but for me seeing them float in air also doesn't help much.
The objects are helpful when checking for overlaps very closely like :
Screenshot-from-2018-06-07-23-16-04.png
Put them back.
Yeah I had a backup :D
Agreed they are hard to see with objects shown but for me seeing them float in air also doesn't help much.
But, you can do this only if you have seen them. For example, I could only see the overlaps at the tires, but seeing them float in air showed the ones below the driver's cab too. Now, I can have a closer look at them with g4 switched on.
yeah that is true!
the floating in air is helpful because the constructor knows where he placed the objects and if some standard view like left/right is used then it can be easy to identify
It was really reasonable to let check_overlaps behave like rtchek in mged. It was, until you had the great idea of allowing to explicitly mention the objects. This extends the possibilities and makes things easier. Sorry, but by implementing this feature messed it up ;) I don't want the old behavior back.
It was really reasonable to let check_overlaps behave like rtchek in mged. It was, until you had the great idea of allowing to explicitly mention the objects. This extends the possibilities and makes things easier. Sorry, but by implementing this feature messed it up ;) I don't want the old behavior back.
haha it was not an idea but for testing purposes before I get into the code of getting objects from view xD. Well if that now counts as a good one then I am good :)
Umm I think my patch for adaptation is ready
documentation one would be ready by tomorrow.
That's OK. I couldn't check them today anyway.
okay cool :thumbs_up:
hey can we rename executable rtcheck to check_overlaps as well for consistency ?
the documentation would be less confusing if that is done, in my opinion.
This doesn't mean however that you can't realize own ideas and improve the behavior.
yes, absolutely this too! apologies if it came across as suggesting you not think about and make things better. sometimes the best solutions come from fresh eyes.
hey can we rename executable rtcheck to check_overlaps as well for consistency ?
we'd have to follow our change policy (see top-level CHANGES file), but that's a bit too specific for my command sense taste.
we did a whole command planning overhaul a while back to plan out how we're going to consolidate the crazy top-level namespace. the intention is to merge the features of rtcheck+gqa+glint+analyze into one command. I don't recall what we settled on, but I'd expect something a little generalized with subcommands like "check overlaps ..." or "analyze overlaps ..." or "evaluate overlaps ..." etc
hey can we rename executable rtcheck to check_overlaps as well for consistency ?
I'm in favor for different names, because I think it could be confusing to have a TCL command and a program with the same name but (slightly) different behavior/parameters. And regarding Sean's comment, the names may be changed later on.
we'd have to follow our change policy (see top-level CHANGES file), but that's a bit too specific for my command sense taste.
humm the CHANGES file is interesting.
I'm in favor for different names, because I think it could be confusing to have a TCL command and a program with the same name but (slightly) different behavior/parameters. And regarding Sean's comment, the names may be changed later on.
Okay got it.
yeah the confusion arising from what you say sound true.
I had this doubt when doing the documentation to whether to explain the options for check_overlaps, because the -h option prints the usage info already.
I think I should do that at least in the man pages
there's a school of thinking that help should be pervasive and in many forms as you don't know where the user is at and they may not know how/where to find help (e.g., that there is a -h option or that there is a manual page).. especially in an inconsistent system like ours
okay got it adding the options everywhere.
The confusion arose from manual for rtcheck command not describing it and saying to refer to rthcheck program's manual for description of the options, but now that we have two different names so doing that is not correct!
@Saran Narayan sorry if I made extra work for you to undo changes or made things confusing. as usual daniel's insights are spot on. having a deterministic command line that works the same inside and out are better than one that does has different usage, though I think we're moving towards there only being one.
saran, would it be hard to make your command take a subcommand (overlaps) as a required argument? that would be useful to set the stage for adding more sub-commands down the road (and it's the same number of chars usability-wise), even if you only are concerned with this one.
oh, and something to think about ...
I'm in favor of a pervasive help too.
there is the possibility for different overlap visualization methods ... the yellow wireframe lines are okay, but certainly not the only way
one of the things the checker script did in a previous form was calculate the overlapping volume via tessellation and then draw that as a solid yellow volume. it was quite effective!
i think I have a picture somewhere...
not suggesting you do that .. just something to keep in mind, that we might not stick with yellow lines or might have other options
sounds interesting and looks good!
options that deal with grid sampling (e.g., the -s size and -n, -w options) are similarly tied to the current implementation -- not the generic conceptual notion of telling a user where there are overlaps
there's (what I believe to be) a far superior method for finding overlaps using quasirandom spherical sampling, halting at convergence or density criteria
even gqa shooting three grids has proven quite inadequate by our professional modelers
The content of the overlay is generated in the log_overlaps() function. I.e. if someone wants to improve it one day, the entrypoint is well determinate.
options that deal with grid sampling (e.g., the -s size and -n, -w options) are similarly tied to the current implementation -- not the generic conceptual notion of telling a user where there are overlaps
I would be in favor of a more generic interface, which is implementation independent, too. But, how much can we break the existing interface (i.e. parameters)?
saran, would it be hard to make your command take a subcommand (overlaps) as a required argument? that would be useful to set the stage for adding more sub-commands down the road (and it's the same number of chars usability-wise), even if you only are concerned with this one.
I think this can be done in the wrapper itself
I would be in favor of a more generic interface, which is implementation independent, too. But, how much can we break the existing interface (i.e. parameters)?
a new command can do whatever we want it to do :)
the only consideration is a practical one that one of the goals of this project is turning that shell script into C (so it can run on windows), with it's specific overlap reporting behavior as that's tied to an entire production workflow in active use
that begs having a command (maybe a separate command? or maybe it's the first implementation guts to this command?) that does what the current does -- but even then doesn't need to expose the parameters of gqa or rtcheck or anything else
it's being called by a gui and is essentially unpublished, so it can change or be whatever is needed
a new command can do whatever we want it to do :)
That's true, but how about a command which shall replace another one? It isn't the goal to have a bunch of commands which do the same thing all a bit different (rtcheck, qga, check_overlaps, ...).
what I kind of thought would happen, in libanalyze terms, is that we end up with an interface that merely generates ray patterns. it doesn't shoot them, just creates them. so rtcheck would call this routine to create a grid of rays based on the current view, gqa would call the same routine, but pass the three orthogonal views. another pattern that could be supported might be "random spherical". this new command could either be the generalization that lets you specify what pattern(s) you want or has some notion of predefined pattern sets (or both).
so the tool only has to concern itself with what patterns it needs to generate and something parallelizes them all (iteratively based on some other criteria)
the goal would indeed be to replace all those commands, so there fundamentally needs to be "some" way to specify a direction vector and generate a pattern (grid) of rays so I can get current rtcheck and/or gqa output
Regarding r71050, the rtcheck command can't just disappear without going through deprecation. It's an ancient tool in production use (within mged) that predates me even joining development.
yup got it, so how to proceed since it has been committed? A commit adding back the rtcheck command?
essentially -- you'll need to reverse-merge at least the rtcheck.c file to undo the delete. that way it will retain the history of edits when it's restored.
something like this http://www.thedumbterminal.co.uk/posts/2012/04/restoring_a_deleted_file_or_directory_from_subversion.html
it might be easiest to undo all of r71050 and then redo the gui changes
s/redo/reapply/
something like this http://www.thedumbterminal.co.uk/posts/2012/04/restoring_a_deleted_file_or_directory_from_subversion.html
well I didn't delete the files, so it should be good, just moved _ged_wait_status from rtcheck to nirt.c
it might be easiest to undo all of r71050 and then redo the gui changes
yup this seems the way to go with the gui changes.
so I should revert using svn merge -r [current_version]:[previous_version] [repository_url]
and then svn commit -m "Revert bad commit r71050"
but this would revert all the commit in between
welp, actually I am not sure :/ svn is kinda new to me
no, that looks right
https://stackoverflow.com/questions/13330011/how-do-i-revert-an-svn-commit
it's not a "bad" commit, so I would change the message to just that you're reverting r71050 and say why (can't delete rtcheck without deprecation)
ok understood, I sent a commit..
humm it got aborted
you're probably out of date -- I just made a few commits
svn up
hehe ok
done :)
BTW since I cannot remove rtcheck. What must be done to the new rtcheck program (that uses libanalyze)?
I had removed the compilation of previous rtcheck from cmakelists in src/rt/ and replaced it with mine.
there is no "new rtcheck program" ... you named it something else :)
that was the check_overlaps command, I also wrote the rtcheck program. https://sourceforge.net/p/brlcad/patches/495/
they're essentially just independent commands at this point. if the new command essentially does everything rtcheck did, then we can schedule rtcheck for formal deprecation. one of us familiar with all rtcheck can do can evaluate what is missing, if anything. the gui menus can still be updated to call the new one because they're functionally the same.
AH, now that's a different question! sorry, misunderstood
what's needed and doesn't exist to answer that question is a regression test -- do you know if it behaves the same, produces the same output?
it should behave the same, I did do unit tests comparing the old rtcheck and new rtcheck.
But output is different. It does not display all the debug information that current rtcheck prints.
debug info?
this is how it would put https://pastebin.com/sHDxtUw3
orignal output of rtcheck https://pastebin.com/aQxYipPY
ah, the setup and shutdown performance metrics... interesting
probably fine, but let me get back to you on it in a few hours. can you show something with more interesting/complicated output, like maybe run on havoc in havoc.g?
old and new
okay will do :thumbs_up:
wow the outputs are different for the pairs themselves! need to investigate..
the count of overlaps is fine
Regarding r71050, the rtcheck command can't just disappear without going through deprecation. It's an ancient tool in production use (within mged) that predates me even joining development.
Reverting the whole r71050 commit looks a bit like an overreaction. Wouldn't it be sufficient to let the TCL rtcheck point to ged_check_overlaps()?
humm I believe we want the rtcheck menu also in Archer under raytrace where I added check_overlaps. So there was a lot of changes to do, so I thought revert was a good way to go.
@Sean
the new rtcheck output:
https://pastebin.ca/4044082 - without -R flag
https://pastebin.ca/4044098 - with -R flag
old rtcheck output:
https://pastebin.ca/4044120 - without -R flag
https://pastebin.ca/4044109 - with -R flag
@Daniel Rossberg
I fixed a big derp with the reg1 and reg2 names being copied
This was what I had before:
BU_ALLOC(op->reg1,char); BU_ALLOC(op->reg2,char); bu_strlcpy(op->reg1, reg1, sizeof(op->reg1)); bu_strlcpy(op->reg2, reg2, sizeof(op->reg2));
this worked for truck.g because all the region names were less than 8 characters, but with havoc the regions names were just "/havoc/".
I fixed it with :
new_op->reg1 = (char *)bu_malloc(strlen(reg1)+1, "reg1"); new_op->reg2 = (char *)bu_malloc(strlen(reg2)+1, "reg2"); bu_strlcpy(new_op->reg1, reg1, strlen(reg1)+1); bu_strlcpy(new_op->reg2, reg2, strlen(reg2)+1);
this looks fine right?
I need to update check_overlaps.c with this.
looks fine to me, gonna commit :)
humm I believe we want the rtcheck menu also in Archer under raytrace where I added check_overlaps. So there was a lot of changes to do, so I thought revert was a good way to go.
If this is the easiest way for you, OK, but you have to commit the parts which were uncritical now separately.
yep got it :thumbs_up:
have to fix the documentation patch too.. without removal of rtcheck related stuffs
Do the functionality part first. Then, describe what you did in the documentation. And yes, if we keep the rtcheck command, then we need to keep its documentation too.
Okay got it.
Your comment on #496
1) Please have a look at the following widgt: mged->Tools->Overlap Tool.
What to do about this? This seems to be broken.
2) Please test the widget Archer->Raytrace->check overlaps->512x512 with an empty view (nothing is drawn).
This I had fixed with a catch statement and print the warning as pop-up message/
1) Please have a look at the following widgt: mged->Tools->Overlap Tool.
What to do about this? This seems to be broken
Maybe you can fix it?
2) Please test the widget Archer->Raytrace->check overlaps->512x512 with an empty view (nothing is drawn).
This I had fixed with a catch statement and print the warning as pop-up message/
For example. (I think there is already a pop-up message, but the only way to close it is to exit the program.)
1) Please have a look at the following widgt: mged->Tools->Overlap Tool.
What to do about this? This seems to be brokenMaybe you can fix it?
Okay I will have a look after all the pending work is done.
2) Please test the widget Archer->Raytrace->check overlaps->512x512 with an empty view (nothing is drawn).
This I had fixed with a catch statement and print the warning as pop-up message/For example. (I think there is already a pop-up message, but the only way to close it is to exit the program.)
Yeah, now the message is not that serious. Like a informational message. If this is not fine then I could print the message in command area.
1) Please have a look at the following widgt: mged->Tools->Overlap Tool.
What to do about this? This seems to be brokenMaybe you can fix it?
Okay I will have a look after all the pending work is done.
:thumbs_up:
2) Please test the widget Archer->Raytrace->check overlaps->512x512 with an empty view (nothing is drawn).
This I had fixed with a catch statement and print the warning as pop-up message/For example. (I think there is already a pop-up message, but the only way to close it is to exit the program.)
Yeah, now the message is not that serious. Like a informational message. If this is not fine then I could print the message in command area.
I don't complain about the message. I complain about that I can't close it. I could exit it only by closing the whole program.
hehe I fixed that. I will show it once, I reapply the changes :)
If you have fixed it for you and will commit the changes later on, I'm fine. I wanted only mention that there is an issue.
yeah thanks for pointing it out. Was a serious bug, like the message would keep on coming back to back.
I was comparing the outputs of rtcheck programs, these are the difference. I was wondering why this happened
https://www.diffchecker.com/iTmDvAKR
left is old and right is new
Looks like the entries are in different order?
but there is one pair that is extra on new rtcheck, which makes the unique count 20
one on line 14,15
humm those are identical pairs
so that condition which checks reg1 == reg2 is not working.. will check it out
Indeed :simple_smile:
It seems to overlap with itself. Shall this be reported?
No that is not it I think, Look at 20,21 lines on both side they also have reg1==reg2
can the outputs vary with each run o_O.. once old rtcheck reported 18 unique pairs now it is reporting 20
https://pastebin.ca/4044821 back to back ran twice. different outputs each time
The order can vary, but the number? You should check it single thread (one processor) first.
okay now it is better :)
https://www.diffchecker.com/Z090oQsR
Hmm, there is still a difference.
However, you should check your semaphores next. I could imagine that there is an unprotected variable.
okay it seems like it
I ran with -R flag and both were identical! :D
oh I think I know why it was showing that slight variation.
The older implementation used a singly linked list and to check for any reverse duplicates it went to the beginning and used the next pointer to check for it: https://pastebin.ca/4045571
to improve this behaviour I used bu_list so it would go from current pair and backwards until it reaches head: https://pastebin.ca/4045594
So I believe the new rtcheck displays correct output :thinking_face:
Since the -R flag outputs are same,
there are 4 times this pair:
/havoc/main_rotor/mr_bldassys/mr_bldassy101/mr_phorn101/r.rot138 /havoc/main_rotor/mr_bldassys/mr_bldassy101/mr_bldroot101/r.bld101
and only one of the : /havoc/main_rotor/mr_bldassys/mr_bldassy101/mr_bldroot101/r.bld101 /havoc/main_rotor/mr_bldassys/mr_bldassy101/mr_phorn101/r.rot138
and checking this : rtcheck.txt
we see that the single pair is at 31st position, where as the one that gave 4 count is at 26th, 27th, 28th and 30th.
The output is different because of the direction of iteration.
I am gonna push three commits for check_overlaps adaptation to Archer and MGED.
One pushed..
it has been awfully slow for me :/
svn: E175002: Commit failed (details follow):
svn: E175002: Unexpected HTTP status 504 'Gateway Time-out' on '/p/brlcad/code/brlcad/trunk/src/tclscripts/archer'
gonna try ssh checkout maybe that will solve the issues
I prepared the docbook changes after reverting any rtcheck command changes I previously did: new_doc.patch
Once the doubt about new rtcheck vs old rtcheck (programs) is clear, documentation would be good to go! Because in this patch I have added -d flag to new rtcheck program.
Just a remark: You should, whenever possible, have only one issue per commit.
For example: The commit for revision 71065 handles two issues.
ohh got it. :sweat_smile:
even those three to commit were hard, I don't know why but commit and checkout seems very slow. Took multiple tries to get one commit pushed. I decided to checkout using ssh, hoping it would improve all this problems, it has been running for 5 hours now :/ still not completed.
Yes, sourceforge has had its days :(
ahh, well GitHub soon I suppose :)
what is your opinion on the difference between the old rtcheck and new rtcheck. It happens because of the direction of iteration of the lists as I had explained earlier.
yay ssh checkout is ready :)
ahh, well GitHub soon I suppose :)
I'm personally not a fan of git. And, especially with such a big repository as BRL-CAD, I'm expecting huge checkouts. Well, let's see.
what is your opinion on the difference between the old rtcheck and new rtcheck. It happens because of the direction of iteration of the lists as I had explained earlier.
Yea, I saw it and thought: okay ;)
hmm okay :thumbs_up:, still need a final answer on whether to replace old rtcheck while I wait for it, I will have a look at the MGED's overlap tool to fix the error.
According to the r61110 g_lint was renamed to glint, so overlap tool was broken because it was expecting g_lint which no longer exists. So renaming g_lint to glint in src/tclscripts/mged/overlap.tcl fixed it.
I was thinking to add getting view information like az, el and size information as default values when the overlap tool is open. Because the defaults 0, 0 and 3.937008in (100mm) for az, el and size is not helpful.
I was wondering on what to do next, I started to read the checker.tcl file to understand it.
According to the r61110 g_lint was renamed to glint, so overlap tool was broken because it was expecting g_lint which no longer exists. So renaming g_lint to glint in src/tclscripts/mged/overlap.tcl fixed it.
Good catch Saran. That warrants a "fixed bug" line in the top level NEWS file with your name on it.
I was wondering on what to do next, I started to read the checker.tcl file to understand it.
I read through what you wrote on the ordering of old vs new rtcheck and didn't fully understand the situation. Is the problem that one of them is not collapsing ordered pairs into the same result? That is A overlaps B and B overlaps A is treated as two instead of one result?
I believe the original intention is to just list such and A and B pairing once, not twice
regardless, users need the shortest useful list of overlaps because the typical workflow is to go through them one at a time for the "important" ones until all overlaps are below some size
if you have that all working with the new rtcheck, I'd think your next step would be integrating the next thing the shell script was doing
According to the r61110 g_lint was renamed to glint, so overlap tool was broken because it was expecting g_lint which no longer exists. So renaming g_lint to glint in src/tclscripts/mged/overlap.tcl fixed it.
Good catch Saran. That warrants a "fixed bug" line in the top level NEWS file with your name on it.
done :)
I was wondering on what to do next, I started to read the checker.tcl file to understand it.
I read through what you wrote on the ordering of old vs new rtcheck and didn't fully understand the situation. Is the problem that one of them is not collapsing ordered pairs into the same result? That is A overlaps B and B overlaps A is treated as two instead of one result?
Humm after actually scribbling the linkedlist on a paper and seeing the iteration it goes through, the direction should not matter, so what I told didn't make sense xD.
So I started debugging and found out why:
Original rtcheck, when it sees that a pair is seen in reverse it breaks and inserts the new pair before that and discards the old pair by setting the new_op-> next = NULL. See lines 191-193 and 209 in rt/viewcheck.c.
It cleverly does this by using a prev pointer which, I thought was kept in the for loop (that checks for reverse pair) just for reaching the end of the list.
Now I need to find a way to do this in bu_list for my new rtcheck.
Fixed! This is the final diff of the outputs for havoc.g https://www.diffchecker.com/aW9UNTRM (left is old and right is new)
I ran with multiview and compared the outputs of 15 runs, there are still some differences in some frames :/ will work on it.
BTW the behaviour of old rtcheck removing the link doesn't seem right to me. That could mean a loss of data
like check this : https://www.diffchecker.com/6eUp2NXS
I have print some extra information for debugging. Left is old and right is new.
Look at line 84 the reverse pair it found was actually at position line 81.
What the old rtcheck did was remove the pairs on line 81 and 82 and inserted the pair at line 95.
What the new rtcheck did was just remove the pair on line 81 only.
old rtcheck just removed the information at line 82 which is not correct IMO.
@Daniel Rossberg please have a look
like check this : https://www.diffchecker.com/6eUp2NXS
I have print some extra information for debugging. Left is old and right is new.
Look at line 84 the reverse pair it found was actually at position line 81.
What the old rtcheck did was remove the pairs on line 81 and 82 and inserted the pair at line 95.
What the new rtcheck did was just remove the pair on line 81 only.
old rtcheck just removed the information at line 82 which is not correct IMO.
I'm confused: Does old rtcheck remove lines 81 and 82 and inserted line 95, or just removes line 82?
When I understood you right, old rtcheck replaces line 82 by line 95, and new rtcheck doesn't do this. But lines 82 and 95 differ by the number of matches, right?
old rtcheck removes pairs at line 81 and 82 to insert the pair on line 95(on left side)
while inserting the new pair on line 85, it detects that a reverse pair (the pair on line 81) exist.
Old rtcheck in attempt to remove this reverse pair, it makes the next
pointer of pair at line 80 to pair line 85. But what it does is it discards the information on line 82 with it. Get it?
what old rtcheck does:
representation
what I did that gave the above diff which I think should be done. Since I am using bu_list it is a circular doubly linked list
representation2
OK, I see what you mean. The prev_ol=op,
in line 189 of src/rt/viewcheck.c was probable unintentional (copy-n-paste error?).
I think it serves the purpose when inserting at line 203, because it inserts new pairs at the end.
but there is already one prev_ol = op on top too at line 174
so either one of them could be unintentional but as you said line 189 seems like a copy paste error
yeah definitely seems like a copy-n-paste error. Because the upper loop either returns when a duplicate is found or reaches the end of the list for insertion
finally same, after removing that error in old rtcheck : https://www.diffchecker.com/2xw8qsYG
So, it's OK now, I suppose?
yeah good to go!
just different ordering without that -P1 flag
if you have that all working with the new rtcheck, I'd think your next step would be integrating the next thing the shell script was doing
We have not yet started with the shell script. Hence I had some basic questions related to the implementation, so that a plan can be made before proceeding.
I am hoping to discuss it tonight with Daniel.
just different ordering without that -P1 flag
This is because the parallel execution introduces an element of chance. Depending on how much time the different threads get from the operating system their results can show op in different orders.
since the overlap tool requires one shell script and one tcl file. And our plan is to combine them into one right?
So would it be like a command or an executable?
how would it be possible to provide GUI like the checker.tcl, with a c/c++ file? like "Wrap C/C++ code to make it callable from Tcl" in this https://wiki.tcl.tk/3474?
Maybe I'm wrong, but the GUI can be still written in TCL/Tk, but it shouldn't call another program but work as mged or Archer by calling libged functions.
by libged you mean MGED commands right? like it does in checker.tcl file line 1093 it calls the draw command :
if [catch {draw -C255/0/0 $path} path_error] { puts "ERROR: $path_error" }
I am confused on why are we even converting the checker.tcl file to a C/C++ file? Aren't tcl files cross-compatible ?
Hmm, I thought the main issue regarding portability is with the shell script check.sh.
yeah the shell script has to be implemented in C. That I agree
the shell script needs a list of objects to check overlaps for.
Probable a mixture of TCL and libged. Maybe, check_overlaps has already everything what you need. However, I haven't looked at check.sh closely yet.
the shell script needs a list of objects to check overlaps for.
This is what the Tk GUI is for?
like I was hoping like to click the Overlap Tool from a drop down menu, which shows a UI to input the objects. Then it runs the check_overlaps command and gives the ouput to the checker.tcl
the script does basically run rtcheck and gqa 16 times with different az/el values ( 0, 45, 90, 135),then using a combination of text processing tools on the output to get just the region names and volume (multiplying max-depth with count). Then sorting and removing any duplicates. Saving this all to a pairing file which the checker.tcl reads
OK, TCL should have the necessary text processing functions build in already.
Left gqa.
A TCL file that calls the check_overlaps command and reads the ouput using regex.
Something like this.
yeah sounds alright to me. Shouldn't we remove the requirement of files? and keep everything in memory when we invoke checker.tcl ?
You should look for gqa and see if there is already a libged function for it. There is already some functionality in libanalyze to support gqa functionality, but I don't know how it's used.
yeah sounds alright to me. Shouldn't we remove the requirement of files? and keep everything in memory when we invoke checker.tcl ?
Definitely!
You should look for gqa and see if there is already a libged function for it. There is already some functionality in libanalyze to support gqa functionality, but I don't know how it's used.
well gqa is already a libged function. Infact the executable version just calls ged_gqa
So, we are fine there :)
gqa uses the libanalyze function add_unique_pair
in libanalyze/overlaps.c to basically check for duplicates and increase the count and update the max depth
So, we are fine there :)
yeah :)
yeah sounds alright to me. Shouldn't we remove the requirement of files? and keep everything in memory when we invoke checker.tcl ?
Definitely!
So I was looking at the usage for check.tcl. It expects check [-F] [overlaps_file]
so the check.tcl file would need some change.
gqa uses the libanalyze function
add_unique_pair
in libanalyze/overlaps.c to basically check for duplicates and increase the count and update the max depth
This is another issue, which Sean has mentioned too, that several functions have the similar functionality to shoot a grid of rays and analyzing the callbacks. This is something you should look at in the next stage, when you are familiar with gqa and did the checker GUI.
This is another issue, which Sean has mentioned too, that several functions have the similar functionality to shoot a grid of rays and analyzing the callbacks. This is something you should look at in the next stage, when you are familiar with gqa and did the checker GUI.
okay got it.
to get started with checker GUI, I will first make a simple UI with a text box and calls the check_overlaps command with the inputed objects.
Got this far, then my ISP went down :(. Using mobile data now.
Screenshot-from-2018-06-15-23-21-34.png
right now I am just getting the values and printing instantly without storing.
I was planning to do something like a database using dicts with keys as reg1, reg2 and volume.
oh just received the mail regarding the first evaluations, thank you for the feedback :simple_smile:
Maybe you can store the overlaps in a list similar to the one you created for check_overlaps()?
Yep, that was the plan. But linked list not there in Tcl. I'll read up and find something to use for it.
This looks fine
http://www.wellho.net/resources/ex.php?item=t245/itlist
For example. Another possibility would be to use a set of lists. Each one for the first object, second object, number of overlaps, depth, etc..
But, the example you found is more elegant.
yeah true, the set of lists was the first thing that came to mind but using objects I would be able deal with them as pairs
and have a common variable containing everything
That is done, I also added a sorting function which is passed to lsort -command
.
Next things to do include gqa command's output, then imitate the behaviour of check.sh of grouping the pairs by name with increasing volume accordingly.
So I was going through src/tclscripts/checker/check.sh
On line 141, I was wondering why keep that line inside the loops that iterate the az, el values.
if line 139 was not appending to the $OBJ.pairings with >>
I would have understood why.
But keeping both line 139 and line 141 is actually redundant, was it a typo?
After adjusting the possible typo in check.sh, and pointing check.sh to the compiled rtcheck instead of prebuilt one.
I was able to get this https://www.diffchecker.com/f4Kn2zVh.
There is minute changes in rounding/decimal places. On left is ouput from check.sh and right is the output of my tool
Well yes, it looks like line 141 should be put after the az-el loop.
Cool thanks for checking it out.
I removed the class and objects now using a nested list. It is way less complex than using classes.
I was testing the overlap tool with havoc.g. It was crashing, to inspect it :
I ran check_overlaps -s1024 -a45 -e0 havoc
it gave a seg fault. Backtrace
I also ran gqa -q -Ao -g10mm,10mm havoc
that too gave seg fault. Backtrace
Running same with rtcheck in terminal is fine : log also for gqa : log. Those extra messages being printed by bu_log is causing the issue in MGED I think judging by the backtrace.
But there are other messages like Trying initial grid spacing: 10 mm in gqa which are printed using bu_log. in libged/gqa.c . Am I missing something here?
Any suggestions to link the tools together removing the file dependency?
Hmm isn't it better to store the overlaps as a file ?
Because it's a lengthy process - running check overlaps 16 times and gqa at 1mm,1mm.
The user shouldn't have to wait everytime the tool is opened, how about display a window asking to either "load previous data" or " create new overlaps data"?
I made some progress with the above idea :)
Now a menu is displayed : overlapmenu.png
The user has three options - 1) Browse for overlaps file, 2) Create new overlap file, 3) if overlap file is found in the directory of the db then user can launch checker directly.
Browse menu is pretty straightforward, once browsed it displays the status and Go is enabled : browse.png
Create new overlaps file launches the tool I made: overlaptool.png
which closes the overlap menu and opens checker when overlaps file is created.
check.png
right now menu+overlap tool is in one tcl file and the checker separate.
I was testing the overlap tool with havoc.g. It was crashing, to inspect it :
I rancheck_overlaps -s1024 -a45 -e0 havoc
it gave a seg fault. Backtrace
I also rangqa -q -Ao -g10mm,10mm havoc
that too gave seg fault. Backtrace
Running same with rtcheck in terminal is fine : log also for gqa : log. Those extra messages being printed by bu_log is causing the issue in MGED I think judging by the backtrace.
But there are other messages like Trying initial grid spacing: 10 mm in gqa which are printed using bu_log. in libged/gqa.c . Am I missing something here?
I can't see anything at hastebin.com. Therefore, I tried it myself and got a crash in Tk_FreeGC() while outputting a "Root solver reported %d intersections != {0, 2, 4} on %s" error message. Because it works with the -P1
option, I'm in favor for a multithread related issue.
I can't see anything at hastebin.com. Therefore, I tried it myself and got a crash in Tk_FreeGC() while outputting a "Root solver reported %d intersections != {0, 2, 4} on %s" error message. Because it works with the
-P1
option, I'm in favor for a multithread related issue.
Then it is a multi thread issue, but unclear on why it happens. It once didn't crash and actually worked so unexpected behaviour can be linked to threads again.
Regarding the usage of files: I prefer to not use files for handing over data from one function to another but to use the direct way. If you want however to use them to save the results of an analysis run for later evaluation its another story and maybe reasonable.
yeah the use of files not only helps to save time spent in finding out the overlaps and there is also a marked file generated by the tool to save progress of resolved overlaps
Then it is a multi thread issue, but unclear on why it happens. It once didn't crash and actually worked so unexpected behaviour can be linked to threads again.
It looks like the log call hook to TCL/Tk isn't protected by a mutex(?)
That looks like it, lemme try protecting it.
@Daniel Rossberg that didn't work, I wrapped with bu_semaphore_acquire(BU_SEM_SYSCALL)
and bu_semaphore_release(BU_SEM_SYSCALL)
It still crashed :/
I also noticed a weird behaviour. Here is how I was able to reproduce it :
1) Run check_overlaps -a45 -e0 -P1 havoc
2) Run check_overlaps -a45 -e0 havoc (without P option)
Then MGED would freeze. The command appears twice:
Another weird behaviour. Steps:
1) Run check_overlaps -a45 -e0 -P1 havoc (7 root solver messages)
2) Run check_overlaps -a45 -e0 -P1 havoc (Only 3 root solver messages)
3) Run check_overlaps -a45 -e0 -P1 havoc (NO root solver messages)
4) Run check_overlaps -a45 -e0 havoc (without P option, NO crashes)
Then for all subsequent runs root solver messages don't show up.
That the root solver messages go away is surprising, that the havoc (or to be more precise the cone primitive) has issues is not, e.g. https://sourceforge.net/p/brlcad/bugs/288
hmm interesting..
I found one more strange behaviour. This time its gqa,
Run gqa -q -Ao -g30mm,30mm havoc and then run gqa -q -Ao -g20mm,20mm havoc. MGED gets frozen with nothing happening.
"processing with grid spacing ..." message appears twice
I found one more strange behaviour. This time its gqa,
Run gqa -q -Ao -g30mm,30mm havoc and then run gqa -q -Ao -g20mm,20mm havoc. MGED gets frozen with nothing happening.
The debugger shows the same reason here: During ray-trace bu_log() is called which results in a segmentation fault in Tk_FreeGC().
It gets stuck here then I have to use crtl+c to show this backtrace on gdb https://hastebin.com/yukugegefa
Hmm, strange.
@Daniel Rossberg that didn't work, I wrapped with
bu_semaphore_acquire(BU_SEM_SYSCALL)
andbu_semaphore_release(BU_SEM_SYSCALL)
It still crashed :/
What did you wrapped with the semaphore?
I wrapped bu_hook_call(...)
on line 85 in src/libbu/log.c, since it seemed the most appropriate.
As that didn't work, I removed the previous changes and tried with call_hook->hookfunc(...)
on line 83 in src/libbu/hook.c.
Also with Tcl_Eval(...)
on line 162 in src/mged/cmd.c since it seemed like the entry to TCL/Tk.
The last two attempts also didn't work and required including bu/parallel.h which seemed not right.
BTW this is the work I have done so far for removing the check.sh.
overlaps_tool.patch
After patching run from mged using overlaps_tool [-F] [overlap_file] (If overlap file is mentioned, then no menu will be shown, directly runs the checker tool)
I wrapped
bu_hook_call(...)
on line 85 in src/libbu/log.c, since it seemed the most appropriate.
The whole bu_log() doesn't look thread safe. See for example at the global variables below the header in src/libbu/log.c.
However, I don't think that these are the reason for the crash. I suppose it is something with the TCL call, but what?
The whole bu_log() doesn't look thread safe. See for example at the global variables below the header in src/libbu/log.c.
Yeah that is true!
even a simple message like bu_log("%s\n",reg1->reg_name);
at the end in overlapHandler(), in check_overlaps.c can crash MGED
BTW this is the work I have done so far for removing the check.sh.
overlaps_tool.patch
After patching run from mged using overlaps_tool [-F] [overlap_file] (If overlap file is mentioned, then no menu will be shown, directly runs the checker tool)
Thanks, I'll have a look at it. I'ts probably the best to proceed with this.
even a simple message like
bu_log("%s\n",reg1->reg_name);
at the end in overlapHandler(), in check_overlaps.c can crash MGED
welp its the same deal here too, with -P1 it works fine!
I've just looked at the mged code to see which semaphores there are used. But, I could find only one call to bu_semaphore_release(), no call to bu_semaphore_acquire().
welp its the same deal here too, with -P1 it works fine!
Yes, it works with -P1
or if the algorithm runs in another process.
I've just looked at the mged code to see which semaphores there are used. But, I could find only one call to bu_semaphore_release(), no call to bu_semaphore_acquire().
By MGED's code where do you mean? Did you try adding bu_semaphore_acquire() before the bu_semaphore_release() ? or could it like it is acquired somewhere else..
Yes, it works with
-P1
or if the algorithm runs in another process.
Didn't understand the algorithm runs in another process. How do we do that?
By MGED's code where do you mean? Did you try adding bu_semaphore_acquire() before the bu_semaphore_release() ? or could it like it is acquired somewhere else..
I did simply a grep on BU_SEM_ in the src/mged directory.
I cannot test setting bu_semaphore_acquire() now because this laptop has only one core (it's an old one).
Didn't understand the algorithm runs in another process. How do we do that?
If rtcheck is called to execute the algorithm it runs in another process. I.e., mged and rtcheck run in different processes.
I cannot test setting bu_semaphore_acquire() now because this laptop has only one core (it's an old one).
Okay I will try to find it and check it out.
If rtcheck is called to execute the algorithm it runs in another process. I.e., mged and rtcheck run in different processes.
oh yeah that is right.
Hmm that is in cmd.c in the function cmdline(struct bu_vls *vp, int record)
but cmdline is never called according to gdb when I run check_overlaps.
So that didn't help but good catch :)
It reminds me on https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=171353
Yeah looks quite similar, and seems fixed with Tcl 8.4.4. But we are running tcl 8.5.19 (? According to NEWS) so should have been fixed.
still why can't we make the entry point to Tcl single threaded
Yeah looks quite similar, and seems fixed with Tcl 8.4.4. But we are running tcl 8.5.19 (? According to NEWS) so should have been fixed.
They fixed another part of Tk (I think it was Tkinter). Tk_FreeGC() and TkGetDisplay() are still the same.
still why can't we make the entry point to Tcl single threaded
Because TkGetDisplay() uses thread specific memory. I.e., the call must come from the main thread, otherwise it won't find the display.
They fixed another part of Tk (I think it was Tkinter). Tk_FreeGC() and TkGetDisplay() are still the same.
hmm that is a bummer!
Maybe we can apply the patch they did for tkinter
hmm running output_hook command first and then running check_overlaps -a45 -e0 havoc seems to solve the issue
but the color of bu_log messages change from red to blue and order also changes i.e it gets printed at the end only
because it deletes the existing hook
the default output_hook command is output_callback
I send an email to the developer mailing list to find out if it's reasonable to fix the Tk binding. It should be possible (in one post they write about "specific APIs for queuing events from one thread into another") but I couldn't find a hint in the Tkinter history yet.
Or, one could do it the same way as in Archer: Send the bu_log() messages to the console (?)
Or, one could allow single thread ged commands only.
For me the Archer way seems the best. After all these are log messages and shouldn't disturb the user.
BTW did you get a chance to look at the overlaps tool I sent yesterday. Since I am still learning Tcl/Tk, I was hoping for some feedback to improve the code.
BTW did you get a chance to look at the overlaps tool I sent yesterday. Since I am still learning Tcl/Tk, I was hoping for some feedback to improve the code.
Hmm, how can I run it?
overlaps_tool
The Go results in wild db_lookup() messages. Is this supposed to be so at the moment?
I have not seen any messages like that
which go button BTW? the menu's or the overlap file one? edit: nvm there is only one GO
db_lookup(v��������5v$) failed: v��������5v$ does not exist db_string_to_path() of 'v��������5v$' failed on 'v��������5v$'
oh that looks bad :O. Can you please tell me how to reproduce this?
It was the truck with g4.
The path to the overlaps file is truck.g.ck/ck.truck.g.overlaps
hmm that sounds alright. Can you check the overlaps file in that folder?
/g4/r136 /g4/r176 305504.8500 /g4/r54 /g4/r74 34041.2306 /g4/r59 /g4/r74 34036.0681 /g4/r69 /g4/r80 33660.9235 /g4/r64 /g4/r80 33648.8302 /g4/r12 /g4/r25 16537.3320 /g4/r20 /g4/r27 16537.3171 /g4/r103 /g4/r105 15767.0500 /g4/r104 /g4/r105 15767.0500 /g4/r103 /g4/r93 14959.0125 /g4/r104 /g4/r95 14959.0125 /g4/r3 /g4/r93 4313.0296 /g4/r3 /g4/r95 4311.5429 /g4/r136 /g4/r179 705.6438 /g4/r178 /g4/r179 705.6438 /g4/r136 /g4/r180 700.0875 /g4/r119 /g4/r93 685.8000 /g4/r119 /g4/r95 685.8000 /g4/r25 /g4/r3 122.2375 /g4/r1 /g4/r27 81.1188 /g4/r1 /g4/r25 81.0265 /g4/r27 /g4/r3 80.9625 /g4/r121 /g4/r122 0.0000
yeah that is as expected!
Screenshot-from-2018-06-28-00-51-08.png
When I restart the overlap_tool it works with the existing file.
yeah it loads from previously generated overlaps file. So with that no wild db_lookup messages?
Right, but after hitting "Create New Overlaps File" I get no window with the overlaps as in your screenshot.
After creating new overlaps it loads the created new overlaps file in the menu and ready to run. That window in screenshot is after pressing the Go button on the menu once it is loaded
I was trying to visualize gqa using geogebra.org with the coordinates printed out with -d option. Now I have an idea of how gqa shoots the ray! I am impressed by how it works like shooting the rays in all 3 axis covering the whole bounding box with uniform grid spacing.
I also like how it skips the repeated rays when refining to a lower grid spacing
fixed a small bug with gqa :) r71101
fixed a small bug with gqa :) r71101
I saw your commit :simple_smile:
After creating new overlaps it loads the created new overlaps file in the menu and ready to run. That window in screenshot is after pressing the Go button on the menu once it is loaded
I've to leave the Overlap Menu and restart it to make the Go button work.
In addition: Why do I have to Browse for a .g file? I'm getting an error when it isn't the same what is currently loaded in mged.
I've to leave the Overlap Menu and restart it to make the Go button work.
I will have a look at it.
In addition: Why do I have to Browse for a .g file? I'm getting an error when it isn't the same what is currently loaded in mged.
It's not for browsing .g file :P I kept it there in case someone needs to load overlaps file from anywhere.
Here is a GIF on how it works for me:
Peek-2018-06-29-01-35.gif
Not sure why you can't run it in one go :/
I made the grid spacing 10mm for making the GIF short. With 1mm it takes some time.
now the text-entry field for objects take focus when it is created and <Return> key is binded to run the check_overlaps button in the overlap file tool
Not sure why you can't run it in one go :/
hmm, maybe I know why it didn't work you. I think you were loading the .g file then running "create new overlaps file".
There is a bug that only loaded the filename once on widget creation only. That is why you had to reload the overlaps_tool.
Thanks for finding it out!, will fix it.
that also explains the wild db_lookup messages :D. I need to think of a way to validate the browsed file. Or should I remove the browse feature ?
It could be handy in case one wants to open overlaps file located elsewhere but then there rises an issue of loading incorrect files or even overlaps file for a different .g file !
well now it restricts browsing only .overlaps file
filetypes.png
got the idea to validate the overlaps file with help of pathlist
command.
All should be okay now :)
overlaps_tool_v2.patch
Made the overlaps_tool such that once it is launched the user can only click/type inside the overlaps tool. I did so that the user cannot modify the database while the overlaps file is generated.
Not sure why you can't run it in one go :/
hmm, maybe I know why it didn't work you. I think you were loading the .g file then running "create new overlaps file".
There is a bug that only loaded the filename once on widget creation only. That is why you had to reload the overlaps_tool.
Thanks for finding it out!, will fix it.
Exactly this was the reason.
You should make clear that there is an either-or in the dialog: Either create a new overlaps file or use an existing one.
I misinterpreted is as: Hit first button, then hit second button, then hit third button.
oh yeah that is a problem, what if I kept the two buttons on one level with a 'OR' label between them
radio buttons would have been nice but there is also the feature of loading previously used overlaps file.
And usually you haven't to browse for the overlaps file because it has a canonical name which will be loaded automatically. On the other hand, you cannot set the file name in Create New Overlaps File.
what do you say we remove the browse button?
there are other problems from loading overlaps like the progress file wouldn't match ( .marked file created by checker tool to keep the resolved overlaps )
And usually you haven't to browse for the overlaps file because it has a canonical name which will be loaded automatically. On the other hand, you cannot set the file name in Create New Overlaps File.
yeah that naming helps load automatically that's why I didn't add option to save overlaps file with a different name
You could use multiple (or a changing) dialogs. The first could have two buttons: Create New File and Load Existing File. If one hits the Create New File the dialog which asks for the group will appear. If one hits the Load existing File button a new dialog which could appear which lets the user select between Use Last File (/home/blablabla) and Browse for Overlaps File. ...
Or one dialog with Create New File, Use Last File (/home/blablabla), and Browse for Overlaps File.
okay both sounds good. So no go buttons.
I liked the one with three buttons because it's less clicks to actually get to the checker tool
If previous file not there then Use Last File would be disabled
I will have the changes ready by the next day. Thank you for the inputs :)
How about something like this :
Looks good. I like especially the tooltips.
Great :)
So I guess it's good to go now and ready to be committed . (?)
Yes, it's a new feature which can be tweaked afterwards if something needs to be improved.
cool! Moving on to the next task. With libanalyze doing the grid setup. I read gqa's code and was thinking how much can be moved to libanalyze. There is this one loop that decides the axis and which calls bu_parallel to run planeworker which actually prepares the ray to shoot.
So if it needs to be moved to libanalyze the arguments could be like the rtip for the models dimensions and it would return like an array or linked list with origin and direction for the rays.
then gqa will be actually shoot the rays by iterating this list preparing the rays
is something like that in mind? because I am a bit confused
Something like that. However, I wonder if passing a large array is the best solution. Another possibility is to pass a function which returns the rays one after the other (i.e. a call-back function).
hmm that sounds better.
what was the basic goal? was it something like merging gqa and check_overlaps. So that we can run check_overlaps within gqa but with a single grid?
The idea was to get a set of functions for all the grid-shooting routines.
oh when we call these functions we can pass like an option to get a single gird or three grids like gqa?
or separate functions for single and three grids
At the moment analyze_overlaps() generates the grid by itself, but how about having grid generating functions which can be passed as parameter to analyze_overlaps(). For example there could be a generator for parallel rays starting from a rectangular grid. Then, gqa could use it three times for the three coordinate axis directions.
didn't understand the part "having grid generating functions which can be passed as parameter to analyze_overlaps()". how would we be passing the function? and from where? libged?
How: As a parameter of analyze_overlaps(). It has currently many other parameters for the internal grid generation.
From where: For example libged. Currently it passes a set of parameters for the internall grid generation. It could be a call-back function instead.
BTW, here is an example for a call-back implementation in C++ (untested ;)
#include <iostream> // typedef for the call-back class NumberProvider { public: virtual ~NumberProvider(void) {] virtual int operator()(void) = 0; protected: NumberProvider(void) {} }; // the function which uses the call-back void PrintNumber(NumberProvider& provider) { std::cout << provider() << std:endl; } // an implemetation of the call-back class Counter : public NumberProvider { public: Counter(void) : NumberProvider(), m_lastNumber(0) {} virtual int operator()(void) { return ++m_lastNumber; } private: int m_lastNumber; }; int main(int argn, char** argv) { Counter counter; for(;;) PrintNumber(counter); return 0; }
Thanks for the example will have a look :)
How: As a parameter of analyze_overlaps(). It has currently many other parameters for the internal grid generation.
From where: For example libged. Currently it passes a set of parameters for the internall grid generation. It could be a call-back function instead.
so we are passing the grid generating function defined in libged as a function pointer to libanalyze. How would this help? why not call this function directly in libged
Yes, it's a new feature which can be tweaked afterwards if something needs to be improved.
Committed : r71106, r71107
Was thinking about the line 301 in TODO file inside tclscripts/checker/
BTW, here is an example for a call-back implementation in C++ (untested ;)
had to brush up my C++ to understand this :D
Was thinking about the line 301 in TODO file inside tclscripts/checker/
Isn't this the one you are working on right now?
Was thinking about the line 301 in TODO file inside tclscripts/checker/
Isn't this the one you are working on right now?
yeah whether to remove this line from TODO
Can you test it on Windows? How does gqa behave there?
Sure can do! Will test it out.
BTW, here is an example for a call-back implementation in C++ (untested ;)
had to brush up my C++ to understand this :D
It was meant as an example that using a call-back mechanism doesn't need to look ugly.
Only an idea, don't know if it's a good one: Stay with the current analyze_overlaps() libanalyze API function, but making analyze_overlaps.c a C++ file and use C++ features there to implement analyze_overlaps() with a separate grid generator. Other programs as gqa could use these directly if transformed to C++ too.
umm not sure about CPP. I am more comfortable with C. :sweat_smile:
But the whole idea of the task still doesn't make much sense to me yet.
The idea was to use a grid generator function which will be passed a callback function to shoot the rays right?
Right, but the separation in the core algorithm and the grid generator needn't to be done in libged.
analyze_overlaps(...) { RectangularParallelRayGenerator gridGenerator(...); analyze_overlaps_core(gridGenerator, ...); }
It's OK to stay with C.
hmm.. Need to clear some more basic questions. I am not sure what are we trying to do here. Yes we need grid generating functions but which command will be using these functions? check_overlaps?
hmm.. Need to clear some more basic questions. I am not sure what are we trying to do here. Yes we need grid generating functions but which command will be using these functions? check_overlaps?
For example, and rtcheck, and maybe gqa.
okay, so what will be doing with analyze_overlaps in rtcheck and gqa? I am confused because gqa does a lot more than just checks for overlaps
gqa would use the same grid generator but with a different base algorithm.
different base algorithm ? you mean splitting the gqa code to libged and libanalyze as we did for check_overlaps?
Yes, and to gtools.
gtools?
Home of the gqa algorithm?
humm. gqa is fully written in libged. in gtools gqa.c only calls the ged_gqa
Ah, I see, it calls ged_gqa().
Not like rtcheck.
yeah that is right
now would that information change anything we have discussed so far?
No, not really. Your plan sounds good.
we would need a different file in libanalyze for gqa right?
Maybe two: One for gqa and one for the grid generator.
ahh so we wont be passing function pointer to grid-generator from libged right? as you had said yesterday
instead call the grid generator defined in the libanalyze
I thought we want to pass the pointer to the grid generator (defined in libanalyze) ton analyze_overlaps() (defined in libanalyze too) in libged's ged_check_overlaps()?
oh, yeah that works. I had misunderstood it before as to define grid generator in libged and pass a pointer to analyze_overlaps in ged_check_overlaps
to summarize it :
1) need a rectangular grid generator function in libanalyze.
2) pass pointer to this function from libged to analyze_overlaps(in case of check_overlaps).
3) call grid setup in analyze_overlaps.
Now grid setup needs function pointer to the function that shoots the rays right. How are we dealing with that?
In analyze_overlaps, analov_do_pixel shoots the rays.
Putting this data in the "context" (the additional variable to the function pointer)?
Hmm not following the idea completely, but I will start coding hopefully then things will be clear to me.
Can you test it on Windows? How does gqa behave there?
I did test it. check_overlaps was crashing due to a derp, but fixed it.
Overlaps_tool works fine now on windows but it freezes in case of havoc. The same bu_log problem there too I think. But it is stranger there, some parts of the messages gets printed then it freezes. I think after some part gets printed the thread gets switched causing the issue.
Of course -P1 works.
@Daniel Rossberg
Today I was going through check_overlaps/rtcheck, on how it produces the grid.
There is one major difference between check_overlaps and gqa.
In the check_overlaps:
There is ability to provide az/el values and shoot from eye. Also the grid formed is on the basis of the size value(zoomed in or out).
If zoomed out, the eye is near and the grid formed is large with high gridspacing.
If zoomed in, the eye goes very far and the grid formed is small with lesser gridspacing.
In gqa, The grid shape and position remains consistent, only grid spacing can be changed.
So I was trying to figure out how can these two use the same grid generator. Like maybe some common arguments.
Just a plotting for three different az/el values and zoom level.
truck.g g4
For example, there could be a function grid_generator()
(don't take my naming too serious) which has a parameter grid_layout
:
struct grid_layout { point_t ray_direction; point_t start_point; point_t grid_x_direction; point_t grid_y_direction; int points_in_x_direction; int points_in_y_direction; int current_x_count; int current_y_count; };
and one or two initializing functions init_grid_layout(<all possible parameters>)
or init_grid_1(<check_overlaps() parameters>)
and init_grid_2(<gqa parameters>)
.
BTW, the grid_layout
would be the context I wrote about.
sounds like a good plan. Thanks for the input.
I got the ray_direction, start_point in the structure.
Are grid_x_direction and grid_y_direction same as dx_model and dy_model which are scaled by cell_width and cell_height of the unit vectors dx_unit and dy_unit or are they just the unit vectors?
points_in_x_direction and points_in_y_direction must be the width and height in pixels right?
Didn't understand the purpose of the last two ints - current_x_count and current_y_count
The following is incomplete!
grid_generator(gl) { int x_index = gl.current_x_count++; int y_index = gl.current_y_count; if (gl.current_x_count == gl.points_in_x_direction) { gl.current_x_count = 0; ++gl.current_y_count; } return gl.start_point + x_index * gl.grid_x_direction + y_index * gl.grid_y_direction; }
You could call them current_~_index
too.
gotcha! I can relate the above code to VJOIN2 (point, workerData->viewbase_model, a.a_x, workerData->dx_model, a.a_y, workerData->dy_model)
Probable ;) I wrote mine with C++ in mind.
great! now I am understanding most of the grid generator function. Thanks a ton for the help :)
Right now confusing me is where would be calling this grid_generator function.
With above code it seems like we can call this right before shooting the rays in case of check_overlaps.
We can get the direction and point with functions returning these values from the structure we initialize .
and in case of gqa just before calling bu_parallel in the views loop we can initialize the structure and use functions similarly in plane_worker to shoot the rays
You could make grid_generator()
accepting struct xray*
as parameter and returning an int
saying if the last point was reached.
oh you mean like passing the a_ray to the grid_generator which would plug in the values for r_dir and r_pt.
Ok then, I will get started on making check_overlaps to use new grid generator function. :)
just different ordering without that -P1 flag
adding the -B flag will let you test parallel while eliminating the element of chance -- it essentially zeros out the random number tables
I was able to get this https://www.diffchecker.com/f4Kn2zVh.
There is minute changes in rounding/decimal places. On left is ouput from check.sh and right is the output of my tool
Sorry to revisit an old point, but @Saran Narayan did you ever figure out what exactly was the source of the rounding differences? differences may be to be expected, but we still need to know what exactly introduced them for this particular feature. your rtcheck diffs seemed spot on, so that even raises the curiosity level.
I was able to get this https://www.diffchecker.com/f4Kn2zVh.
There is minute changes in rounding/decimal places. On left is ouput from check.sh and right is the output of my toolSorry to revisit an old point, but @Saran Narayan did you ever figure out what exactly was the source of the rounding differences? differences may be to be expected, but we still need to know what exactly introduced them for this particular feature. your rtcheck diffs seemed spot on, so that even raises the curiosity level.
@Sean Yes I did. I believe it has to do with awk printing the size value after multiplying the length with depth. It does this rounding that limits output to six digits (like 0.123456, 1.23456, 12.3456, 123.456 ...) see https://www.diffchecker.com/EY0VNKhF on the left is the output of awk multiplying it in check.sh and on the right is the output of tcl multiplying it in my script.
just different ordering without that -P1 flag
adding the -B flag will let you test parallel while eliminating the element of chance -- it essentially zeros out the random number tables
Thanks! I didn't know about the -B flag. It was not there in rtcheck's man. I would need to add this to my current rtcheck.
Here is my progress :
4_07_progress.patch
There is a lot of things that can be removed. Its WIP :)
Here I initialized the grid normally without any functions. What exactly does the initializer function take as parameters? are they similar to check_overlaps's grid_setup ?
Why has analyze_overlaps() still so many parameters?
yeah I am removing them.
OK :)
APP.a_rbeam = 0.5 * viewsize / width
because of this line I have to pass viewsize and width
This means, that a_rbeam is related to the grid cell size?
yeah 0.5 * cell_width
I could pass cell_width in the grid_context then it would be fine.
Then, there should be a function like double rectangular_grid_cell_width(struct rectangular_grid* grid)
, but how. It would be easier to do this with C++, but this isn't a real alternative here ...
humm that is right! I did not think that way
You could use a table of function pointers like
struct grid_generator { int (*next_ray)(struct xray *ray, void *context); double (*cell_width)(void *context); };
yeah this should work. But it could be a bit messy..
ah but with this we can reduce the parameters further.
struct grid_generator { void *context; int (*next_ray)(struct xray *ray, void *context); double *cell_width; };
something like this would be needed right? CMIIW
oh nevermind got it!
Okay that is done. Thanks for the idea :)
Now that we are doing grid setup completely on libged I should move rt_prep_parallel to libged
struct grid_generator { void *context; int (*next_ray)(struct xray *ray, void *context); double *cell_width; };something like this would be needed right? CMIIW
I personally would like to have the two functions, because I don't like redundant data. It was even hard for me to accept the rectangular_grid.total_points
.
Now that we are doing grid setup completely on libged I should move rt_prep_parallel to libged
Ohh, no? For me this is an analyze_overlaps() internal matter.
Now that we are doing grid setup completely on libged I should move rt_prep_parallel to libged
Ohh, no? For me this is an analyze_overlaps() internal matter.
humm I thought about it because I saw my comment above rt_prep_parallel saying it needs to be run before grid_setup.
yeah it is not need.
I will keep it in libanalyze
I tried to implement the changes in gqa.
5_07_progress.patch
I don't know if it's wise to do these changes on gqa while it's still in libged (and not in libanalyze).
You are setting up the grid there directly, but then, you forget that you know struct rectangular_grid and introduce two new functions grid_x_index() and grid_y_index(). I may need some time to form an opinion ...
Yeah I thought of it as an attempt before moving it fully to libanalyze.
BTW What are benefits of moving gqa to libanalyze ?
Shouldn't be vmath.h included in analyze.h?
Shouldn't be vmath.h included in analyze.h?
yeah that sounds good. But all the other libanalyze files have them included separately.
BTW What are benefits of moving gqa to libanalyze ?
Same reason as for rtcheck/analyze_overlaps(), to get the basic algorithm separated in a function which could be used in other contexts too?
Hmm, however, I'm not sure about this.
if it were like different commands for like overlaps, volume, weight then having one common algorithm that does basics would have made sense in my opinion. But it is already aggregated together as one command.
Right.
I thought about the grid_x_index() and grid_y_index() thing because I don't like it. They feel somehow artificial (this is subjective, I know). The reason why they are there is that the grid can be refined and already computed grid points shall not be computed again. (Right?) It would look better if there would be a function like refine_grid_in_between which sets up the grid structure in a way that it behaves so.
The reason why they are there is that the grid can be refined and already computed grid points shall not be computed again. (Right?) It would look better if there would be a function like refine_grid_in_between which sets up the grid structure in a way that it behaves so.
Yeah for refinement and as well for app.a_user line 1379 (unedited gqa.c) which is used in hit():1223
But a function to refine would look good.
hmm I didn't know what to do next. Since we weren't sure about moving gqa to libanalyze.
I just changed the rtcheck program I wrote earlier to use the new grid generating function.
Maybe you could integrate the grid generator in check_overlaps first.
I already did that right? Like did the changes for check_overlaps + analyze_overlaps
Yes, in the patch you send me it was contained.
@Sean What do you want?
Yes, in the patch you send me it was contained.
I will commit those changes then. Will not do those extra functions I added for gqa.
I'll have more time to look into your matter at the weekend. This week was very busy.
Okay got it. Thanks :)
I had added a folder to libanalyze as GridGeneration. So when I commit by svn commit src/libanalyze/GridGeneration/ -m "..."
should work right?
well I committed but I kinda mixed it up :/
I didn't realize I was committing the changes for analyze_overlaps too in include/analyze.h in r71117
I could compile and run it, i.e. it should be OK now.
From your perspective, would it make sense to use the grid generator in gqa?
Would it make sense to replace gqa by a function using analyze_overlaps() because it give you a better control on analyzing the overlaps?
In your proposal there is an image of your prototype for an improved object selection. Do you still want to use it in overlaps_tool?
Do you have other ideas of how the overlaps_tool could be improved?
Currently, the left region is displayed in red and the right in blue. But, when I draw e.g. g4 first, it will be displayed in red too, which makes it impossible to see where the two overlapping regions are. Only the right/blue one can be identified.
Why can't I check the overlaps between /g4/r25 and /g4/r12 of truck.g in overlaps_tool?
From your perspective, would it make sense to use the grid generator in gqa?
Well Sean once told me that libanalyze was like a place for common tools used by these analyzing programs like rtcheck, gqa. So having and using grid generator does make sense to me.
In case of gqa, for rectangular grid it does not seem to help much but if there were more types of grid generating functions. Then having a common place for these grid generating functions and using them does make sense.
Would it make sense to replace gqa by a function using analyze_overlaps() because it give you a better control on analyzing the overlaps?
well gqa does a lot more than just checks for overlaps. So something like analyze_overlaps cannot replace it.
Yeah the better control of analyzing just some regions is better but then with gqa shooting three grids, all overlaps are found.
I would think gqa as a tool to check for overlaps as a whole and check_overlaps as something that can be used to analyze them in detail.
In your proposal there is an image of your prototype for an improved object selection. Do you still want to use it in overlaps_tool?
Yeah something like that is good. Like having the tree displayed would give a better idea of the database.
Do you have other ideas of how the overlaps_tool could be improved?
Currently, the left region is displayed in red and the right in blue. But, when I draw e.g. g4 first, it will be displayed in red too, which makes it impossible to see where the two overlapping regions are. Only the right/blue one can be identified.
Yes that is true that having the colors mixed up is bad.
About the ideas well the TODO list is full of ideas :D. I initially had proposed to improve checker_tool by doing those TODO task but then I was told that converting the unix script to a command was higher priority. Well I could start with that next and try to do those TODO tasks.
Why can't I check the overlaps between /g4/r25 and /g4/r12 of truck.g in overlaps_tool?
It worked for me I could display them on screen. But subtracting wont work unless you launch the overlaps_tool with -F option.
With -F option we are displayed a warning message:
WARNING: Running with -F means check will assume that only the first unioned solid in a region is responsible for any overlap. When subtracting region A from overlapping region B, the first unioned solid in A will be subtracted from the first unioned solid in B. This may cause the wrong volume to be subtracted, leaving the overlap unresolved.
@Sean Yes I did. I believe it has to do with awk printing the size value after multiplying the length with depth. It does this rounding that limits output to six digits (like 0.123456, 1.23456, 12.3456, 123.456 ...) see https://www.diffchecker.com/EY0VNKhF on the left is the output of awk multiplying it in check.sh and on the right is the output of tcl multiplying it in my script.
Ah, yes that makes sense. Could be confirmed by using printf in awk instead of print, with the same print specifier, but it certainly makes sense. Thanks!
Thanks! I didn't know about the -B flag. It was not there in rtcheck's man. I would need to add this to my current rtcheck.
It's really only intended as a developer option, but it probably should be documented better somewhere. I think it's in rt's manual page, so it would make sense if it were on all of the rt* tool docs.
BTW What are benefits of moving gqa to libanalyze ?
Same reason as for rtcheck/analyze_overlaps(), to get the basic algorithm separated in a function which could be used in other contexts too?
Hmm, however, I'm not sure about this.
Not only this. At heart, a main distinction of rtcheck vs gqa is time vs quality. Generalizing their behavior into library routine(s) is intended to get us closer to controling time vs quality more easily. When you consider what check.sh does, it is essentially sacrificing time for even more "quality" -- more overlaps reported. In a near future, we want even more advanced methods that run faster (time, e.g., quick real-time checking) or give better results (quality, e.g., spherical sampling or brep CSG evaluation).
@Sean What do you want?
Your entire discussion over the past few days has been SPOT on, same considerations I've thought through myself. I think the answer could just fine in any number of directions, and a good path is hopefully made self-evident with focus being on the short-term goal of doing what check.sh is doing from C/C++ code without exec's and refining away code duplication from there.
Beyond that, the decision is really up for discussion or just try something so we can have "some time to form an opinion" :)
I think it would be good to think beyond gridded sampling methods -- like s/grid_generator/ray_generator/ where you still have an iterator that returns the "next" ray, but it might not be limited to a grid pattern. The difficulty will likely be how to control parameters of the pattern.
Would it make sense to replace gqa by a function using analyze_overlaps() because it give you a better control on analyzing the overlaps?
well gqa does a lot more than just checks for overlaps. So something like analyze_overlaps cannot replace it.
Yeah the better control of analyzing just some regions is better but then with gqa shooting three grids, all overlaps are found.
I would think gqa as a tool to check for overlaps as a whole and check_overlaps as something that can be used to analyze them in detail.
I've just realized that my question was missing an important addition, namely "in the overlaps_tool". I didn't meant it in general.
Why can't I check the overlaps between /g4/r25 and /g4/r12 of truck.g in overlaps_tool?
It worked for me I could display them on screen.
When I write for example /g4/r25 in the Object(s) field of the overlap_tool I get the error message "Unrecognized object names: /g4/r25".
I've just realized that my question was missing an important addition, namely "in the overlaps_tool". I didn't meant it in general.
Well I don't get the "better control" on analyzing the overlaps in overlaps_tool. To the end user it is just running the commands and listing the overlaps. As a developer yes I had better control in specifying the az/el values but then at the end I had to run it for 16 different combinations of az/el values to get an overall count which gqa does for me with just one command.
When I write for example /g4/r25 in the Object(s) field of the overlap_tool I get the error message "Unrecognized object names: /g4/r25".
Humm I am using the t [obj] command to check if the object actually exists in the db. And running t /g4/r25 gives me a db_lookup error.
the t command works for t r25.
What if I use pathlist command? That works with both g4 and /g4/r25
PS: I am using the pathlist command for verifying the overlaps file browsed is correct or not.
paths command also works
Beyond that, the decision is really up for discussion or just try something so we can have "some time to form an opinion" :)
I think it would be good to think beyond gridded sampling methods -- like s/grid_generator/ray_generator/ where you still have an iterator that returns the "next" ray, but it might not be limited to a grid pattern. The difficulty will likely be how to control parameters of the pattern.
First, you have a point here: It's in fact a ray and not only a grid generator. If Saran agrees too, he could rename it some day.
And regarding the not being limited to a rectangular pattern: This was a goal of this reorganization of the code too. Currently, the ray generator has only two functions: next_ray()
and grid_cell_width()
, where grid_cell_width()
has in fact the meaning of "ray thickness". This is still very general and could be applied to patterns starting from a sphere or randomly in 3D space or ...
gqa has an additional requirement: It wants to control the fineness. There Saran's first attempt relies on the rectangularity of the grid. But, one could think of a ray generator function refine_grid()
, which abstract this for any kind of grids.
However, currently the libanalyze analyze_overlaps() would work with any kind of ray generator whereas libged's check_overlaps relies on the rectangular shape (its input values are designed for this only).
To sum our discussion up, I see the following tasks as the next development steps:
Any comments?
paths command also works
Liked this, because it threw lesser error messages in validating the overlaps file. With pathlist it gave a big error message in MGED's command window.
And now it should be fixed overlaps_tool_objects_fix.patch
To sum our discussion up, I see the following tasks as the next development steps:
* Better object selection via GUI in overlaps_tool similar to the prototype in the proposal.
* Go through the ideas in the TODO for improving the checker_tool and implement some of them, etc..
* Move the core algorithm of gqa to libanalyze and make it work with a generalized ray generator.Any comments?
Okay I will start with the better object selection. Will do some reading on how MGED's geometry browser GUI works.
gqa has an additional requirement: It wants to control the fineness. There Saran's first attempt relies on the rectangularity of the grid. But, one could think of a ray generator function
refine_grid()
, which abstract this for any kind of grids.
gqa's refinement could even be simply embedded in the natural progression of "next ray" where its pattern is an X grid, a Y grid, a Z grid, an X grid with a different thickness (and offset), a Y grid at the new thickness, etc.
However, currently the libanalyze analyze_overlaps() would work with any kind of ray generator whereas libged's check_overlaps relies on the rectangular shape (its input values are designed for this only).
the abstract concepts I'm hearing are ray creation/generation (e.g., grid, 3grid, spherical, etc), dispatch pattern (e.g., sequentially, random, hilbert, etc), and evaluation method (e.g., overlaps, volume, surface area, etc)
I was able to get the geometry listed, using some code from Geometry browser. Changed the double click to add to objs list and color it yellow.
That got me thinking, the overlaps tool runs the check_overlaps and gqa on per objects from the entry box like this :
foreach obj $_objs { $this runCheckOverlaps $obj $this runGqa $obj }
This is how the check.sh also behaved. So giving individual regions in the _objs list won't make sense like /g4/r25 /g4/r12
this would make the tool run check_overlaps /g4/r25
and gqa /g4/r25
then check_overlaps /g4/r12
and gqa /g4/r12
. Both regions individually would give zero overlaps.
So should I change the behaviour to something like this:
$this runCheckOverlaps $_objs $this runGqa $_objs
So that it would run the tools like check_overlaps /g4/r25 /g4/r12
and gqa /g4/r25 /g4/r12
hence get overlaps between the regions.
This is confusing me:
mged> gqa -Ao -g1mm,1mm /g4/r25 /g4/r12 Trying initial grid spacing: 1 mm Using grid spacing lower limit: 1 mm Processing with grid spacing 1 mm 1241 x 688 x 864 /home/sharan/brlcad/mod_ssh/src/libged/gqa.c:1461 Didn't find object named "g4/r25" in 2 entries /home/sharan/brlcad/mod_ssh/src/libged/gqa.c:1461 Didn't find object named "g4/r12" in 2 entries NOTE: Stopped, grid spacing refined to 0.5 (below lower limit 1). Summary (1mm grid spacing): list Overlaps: /g4/r12 /g4/r25 count:727 dist:363.945mm @ (7443 833.03 1093)
it says it can't find objects but then reports the overlaps.
Progress so far :
Peek-2018-07-10-01-42.gif
it says it can't find objects but then reports the overlaps.
@Saran Narayan that is odd. looking at the code, I think you found a bug -- when an object isn't found, find_cmd_line_obj() returns GED_ERROR which is a 1 ... which is a valid object table entry (but wrong!). want to try and fix it?
umm yeah I am trying to fix it but figuring out how it works, gqa expects the objects like g4 or havoc as entry, anything like /g4/ or g4/ throws that error message.
@Saran Narayan that is odd. looking at the code, I think you found a bug -- when an object isn't found, find_cmd_line_obj() returns GED_ERROR which is a 1 ... which is a valid object table entry (but wrong!). want to try and fix it?
Here you say 1 is valid but wrong and in the FIXME comment you said the type is not valid and value is correct.
So the fix would be returning a 'int' with value as 1 ? But that doesn't fix the Didn't find object messages.
umm yeah I am trying to fix it but figuring out how it works, gqa expects the objects like g4 or havoc as entry, anything like /g4/ or g4/ throws that error message.
Well yes, because you can see in find_cmd_line_obj() that it does a simple iteration over the arguments and compares against all regions (®p->reg_name[1])...
since those are region names and not paths, they'll never match a path, so find_cmd_line_obj() returns an index of 1 (i.e., GED_ERROR... which is wrong/bad).
Here you say 1 is valid but wrong and in the FIXME comment you said the type is not valid and value is correct.
So the fix would be returning a 'int' with value as 1 ? But that doesn't fix the Didn't find object messages.
I didn't say the value was "correct". I wrote that the value (1) could be a _valid_ index and in your example it is a valid index. right now, the return "type" of that function is an index into an array (see where find_cmd_line_obj() is called) ... so returning anything non-negative is probably going to be bad... do you see why?
@Saran Narayan any progress?
Well yes, because you can see in find_cmd_line_obj() that it does a simple iteration over the arguments and compares against all regions (®p->reg_name[1])...
since those are region names and not paths, they'll never match a path, so find_cmd_line_obj() returns an index of 1 (i.e., GED_ERROR... which is wrong/bad).
yeah I did see this. In the call to find_cmd_line_obj()
pointer to reg_name[1] is passed which cuts off the front "/" in the region name and inside the find_cmd_line_obj()
it replaces the next occurance of "/" with "null termination".
So from /g4/r1 it becomes g4/r1, then the next "/" is replaced by null termination making it just g4. which matches the object, (i.e g4)
@Saran Narayan any progress?
I had made some progress with the overlap tool- added ability to batch add/remove elements into the objectlist with wildcards, but nothing related the bug fix
I don't know what should be the expected behaviour in case if paths are supplied in cmd-line arguments.
should it be like for input gqa /g4/r25 /g4/r12
on call to find_cmd_line_objs()
for /g4/r25 it must return 0 and for /g4/r12 it must return 1
According to current behaviour it is like linking the objects with the regions i.e /g4/r1, /g4/r2 all return the index of g4 in the obj_table.
so returning anything non-negative is probably going to be bad... do you see why?
yeah that is right because it would match incorrectly. Should the fix be something like this? gqa_fix.patch
Today's progress regarding overlaps_tool's objection selection:
the top entry can also be used to add individual items as well ( not shown in the gif )
now I think what's left is to add Check Tops and Clear Selection buttons.
I don't know what should be the expected behaviour in case if paths are supplied in cmd-line arguments.
what happens if you "draw /g4/r25 /g4/r12" and "gqa" in mged? probably the expected behavior ;)
yeah that is right because it would match incorrectly. Should the fix be something like this? gqa_fix.patch
This looks good to me except that you left the fixme comment ;)
what is the default selection? if something is drawn? if nothing is drawn?
@Saran Narayan I haven't seen you commit anything recently ... at this point, you should be committing daily, throughout the day while you work. each one of the "today's progress" postings here should have been preceded by at least 1 or 10 commits. don't be shy ;)
also, regarding r71117 ... if you're going to introduce anything into public headers in the top-level include/ directory, they need to be documented with /** */ doxygen comments
what happens if you "draw /g4/r25 /g4/r12" and "gqa" in mged? probably the expected behavior ;)
I get the same behaviour :thinking_face: for gqa -Ao after drawing /g4/r25 and /g4/r12
what is the default selection? if something is drawn? if nothing is drawn?
right now for both the default selection is nothing. On previous iteration of my overlaps_tool I had made it get the currently drawn objects using the who command. I would restore that behaviour here too.
@Saran Narayan I haven't seen you commit anything recently ... at this point, you should be committing daily, throughout the day while you work. each one of the "today's progress" postings here should have been preceded by at least 1 or 10 commits. don't be shy ;)
Okay got it!. Will commit regularly.
Committed lots of things. Now its up to date with my progress.
Tested it out on windows 10. Seems to be working alright.
Woot! Looking good. So where do we stand on the original script? Do you do everything it does now?
Not really there are two things missing:
1) dry_run with -d option : this was not feasible because I am dealing with the overlaps in the memory and it doesn't write the intermediate files to the disk like rtcheck program does.
2) There is a block code disabled with a false flag in check.sh at line 264, but it deals with the plot files so again as I am using check_overlaps command instead of rtcheck program it does not write any plot files.
@Saran Narayan 1) I don't think dry_run is important ... I think that was just to make sure the script was iterating correctly and that it wasn't going to explode into too many rtcheck runs if the iteration wasn't right or some size changed
2) can also be ignored for now -- that was a means to generate visualization aids for resolving overlaps. there's other things we can do there that would be better
SO ... then it sounds like you're there or close to it? does check_overlaps (without the GUI) still write out an overlaps file?
what's the usage like?
(command-line usage)
check_overlaps does not write an overlaps file.
(command-line usage)
do you mean from terminal like we can run rtcheck or gqa?
usage inside MGED would be check_overlaps [options] [object(s)]
I mean from the mged command line
so yeah, what are those options :)
options:
-a #, -e #for the az/el values
-s # for size value
-d for printing information like the eye_pos, orientation, grid size...
- w #, -n # for mentioning the width/height
-g # ,-G # for cell width/height.
-P# to set processors used
-V# to set pixel ratio (width/height)
if the options and objects are not mentioned then default values are used -a 35 -e25 -s 512
if objects are mentioned but not options again the default values are used
but if both are not mentioned then it takes the view information runs the tools on the visible objects
so essentially it has rtcheck's usage
yeah basically a replacement for the rtcheck command with the added option of mentioning the objects
you can specify objects to rtcheck
not in MGED
well you have to mention the database too
Screenshot-from-2018-07-11-23-26-29.png
I have truck.g in the bin folder for quick running and it shows this if run rtcheck with objects mentioned
it does find overlaps
that's not the right usage for inside mged
what is it then? running rtcheck g4 also fails
there's rtcheck the application, and rtcheck the command -- they're similar obviously as one wraps the other, but different expectations
the command does not take a .g file -- it uses whatever is open in mged
the app takes a file and list of objects
in mged, it defaults to objects displayed, but will also take a list of objects
the app takes a file and list of objects
yeah. The rtcheck command uses the -M option of the app to feed the parameters and the objects
what's probably confusing matters is the rtcheck command in mged does try to detect when it looks like you're trying to override the defaults (like specifying a different .g file) but it probably doesn't do a very great job at it as that's not common usage
so here's where it gets interesting.
from a design perspective, I've been enjoying listening to you and daniel talk through this new overlaps interface because you have a fresh perspective
detecting and resolving overlaps is one of our most central and frequently used features, and one of the few features that I dare say we handle considerably better and faster than the big-name commercial CAD systems
anyways, we're at a point now where we need it to be even better because we've had this accumulation of features go in three different directions for years
rtcheck, glint, and gqa
there was an old overlap resolution GUI that simply ran rtcheck and let you iterate through the pairs, subtract left from right or right from left or raw edit the combs -- it was terrible
there's the new one that you've been working with that runs lots of rtchecks and also gqa (which is essentially three more axis-aligned rtchecks), presents them in priority order, and provides a workflow interface for fixing them -- this is not terrible
there was an old overlap resolution GUI that simply ran rtcheck and let you iterate through the pairs, subtract left from right or right from left or raw edit the combs -- it was terrible
humm it still exists right? under tools->overlap tool
however, we still have rtcheck and gqa as rather different tools, and now a check_overlaps command that is highly related but also dissimilar
yes, still exists but entirely unused by anyone
it'd be great to get rid of rtcheck and gqa and glint ... replace them with one tool that checks geometry for issues, but that tool definitely does not (or at least should not) have rtcheck's usage as the prima fascia interface to users
oh that is where you were going with check as command and overlaps as sub-command. So that we can have other sub-commands like what gqa offers?
and how would we have a different interface to the user? If these tools were to be merged we can have like a grid pattern option where single grid would mean it shoots rays like rtcheck and 3 axis aligned grid which would shoot the grids like gqa does.
But we still need some of the options of rtcheck like grid spacing, az/el values.
I am not really sure what glint does because I have not read the code for glint or used it before, from the man page it seems very similar to rtcheck. But it reports other information like vacuums and air region problems.
So those other functions/information can be like options or sub-commands.
oh that is where you were going with check as command and overlaps as sub-command. So that we can have other sub-commands like what gqa offers?
bingo (and even more suboptions like glint offers)
hmm then maybe we can have all the algorithms of these three programs in libanalyze. Then have one common driver that calls these libanalyze functions
breaking it down, there's two or three fundamental operations going on -- we're 1) figuring out where there is and is not geometry and 2) asking some question(s) about that geometry and 3) presenting answers to those questions in some meaningful way
rtcheck for example, 1) shoots a rectangular grid of rays to get partitions, 2) figures out and keeps a list of where there are overlapping partitions, 3) presents that list as sets of ordered pairs with their quantities of overlap
gqa for -Ao (overlaps) is the same for 2 and 3 but for (1) it recursively shoots 3 axis-aligned grids of rays.
glint is the same as rtcheck for 1/2/3 but has a lot more options for what it will detect during (2) and has had little attention to the quality of (3)
rtcheck is slightly better at (3) than gqa, but then gqa is far better at (1) and has a lot more options for (2)
thinking of overlaps in particular, there are two additional methods for (1) that we'd like because they'd likely be quite superior -- spherical sampling and surface intersection testing
the first is ray tracing, but the latter (surface intersection testing) isn't even ray tracing, which means (2) would have to be completely different to support it
have I lost you yet? :)
I understood the analogy of the three fundamental operations. Not so much sure about the two new method you are suggesting :D
so the next question is really just how much of 1/2/3 is libanalyze's job and it's probably either just 1 or 1 & 2
from a callback perspective, it's possibly even just 2 for libanalyze, with 1 being registered by the caller as a callback
so an rtcheck-style application might decide to shoot a grid of rays, handing each one back sequentially for the next ray callback()
i really liked the "next ray" concept that you and daniel worked out -- that iterator concept is spot on
that way there can be some predefined patterns (like rectangular grids) or something far more advanced like an adaptive refinement sampling
yep and with something like nextray we can get xray* preped for the patterns
we can have different iterators for different patterns which are registered by libged
these concepts overlap heavily with rendering too, and there you even want to separate the sample ordering since it can have a great effect on performance
for example, I might want to shoot a grid of rays one line at a time from top to bottom or bottom to top, or maybe in little 4x4 postage stamp sets, or maybe randomly one ray at a time, or ...
okay now I understand the patterns you were talking about. I remember tweaking these rendering options to get the max performance in Blender.
in rendering terms, these are typically handled as "camera properties" which doesn't fit well in libanalyze's world but certainly relates since we use render-style analysis methods
yeah got it..
so all that discussion, what's next?
well since I am done with overlaps file tool. I'm ready for the next task. Which would be merging these commands. So to start with I was hoping to move gqa's algorithm to libanalyze?
I need to figure out what things must be moved to libanalyze
that sounds good to me (and tricky)
have you tested how check_overlaps compares with rtcheck in terms of parallel performance?
from a commit you made the other day, it looked like you ripped out most of the code related to parallel chunking
yeah that is true. It has gotten a bit slow now after introducing the grid generating function for sure. But I didn't test it extensively.
it would be good to sort that out before moving on to gqa, just to make sure it's not like 2X-10X slower -- something that might stop people from using it outright
should also make sure the GUI is well-behaved for real geometries -- like how it behaves if you run on havoc in havoc.g
it would be good to sort that out before moving on to gqa, just to make sure it's not like 2X-10X slower -- something that might stop people from using it outright
alright I would think something to introduce the parallel chunking.
if something is slow, there needs to be some visual indication while it's working, something to indicate it's not stuck
alright I would think something to introduce the parallel chunking.
No no, that wasn't the point -- that's a complex topic and could consume the remainder of gsoc. it just requires checking the performance, doing a quick comparison.
alright I would think something to introduce the parallel chunking.
No no, that wasn't the point -- that's a complex topic and could consume the remainder of gsoc. it just requires checking the performance, doing a quick comparison.
haha sure that I can do :)
there's no point doing anything if performance isn't a problem
you just may need to comment out the gqa bits or compare it with the shell script if you make it do the same work
if something is slow, there needs to be some visual indication while it's working, something to indicate it's not stuck
which GUI are you taking about here? the command window when running check_overlaps or the one when creating overlaps with overlaps_tool
you just may need to comment out the gqa bits or compare it with the shell script if you make it do the same work
hmm got it but I can't really run the commands on havoc due to the tk bug. It would crash if I run it with multithread
command window is CLI, not GUI
hmm got it but I can't really run the commands on havoc due to the tk bug. It would crash if I run it with multithread
I didn't fully get what the problem was when you and daniel were talking about that last... which Tk bug is there that affects your tool, but doesn't affect gqa/rtcheck? :)
or why if you know that
oh it affects gqa and check_overlaps. So it ultimately affects the tool.
it does not affect rtcheck because it runs in a different process
on a very loosely related point -- commit 71118. this looks problematic to me in that you are semaphore locking around every ray. that's bad.
I know the old code locked around every chunk -- that was also bad, slightly less bad, but still not good. if you can find a way to do it lockless, that would be best.
if it doesn't work in parallel, there may be other problems
well do you have any suggestions for it :) and yes it does not work in parallel.
having multiple threads accessing the function can get unexpected results
so what's different?
gqa run inside mged doesn't exhibit a problem, or are you saying it does?
yes running "gqa -Ao -g10mm,10mm havoc" inside MGED gives a crash.
:scream:
that would be a release blocker
checking...
yeah it is critical bug. And running "gqa -Ao -P1 -g10mm,10mm havoc" works fine.
hm, that seems to be working just fine here
7.27.0 recent build, on Mac
does it print extra messages?
I'm not seeing anything extra -- looks like it should
I was testing the overlap tool with havoc.g. It was crashing, to inspect it :
I rancheck_overlaps -s1024 -a45 -e0 havoc
it gave a seg fault. Backtrace
I also rangqa -q -Ao -g10mm,10mm havoc
that too gave seg fault. Backtrace
Running same with rtcheck in terminal is fine : log also for gqa : log. Those extra messages being printed by bu_log is causing the issue in MGED I think judging by the backtrace.
But there are other messages like Trying initial grid spacing: 10 mm in gqa which are printed using bu_log. in libged/gqa.c . Am I missing something here?
this is what I had
what does ldd mged show?
at a glance, it looks to me like maybe a library mismatch
also be good to see how mged was invoked
https://hastebin.com/xupewumisu.go
also be good to see how mged was invoked
I don't get it. I just ran ./mged
from build/bin
humm the 7.26.4 release build also crashes for me.
what's the backtrace?
backtrace of the crash?
yes
the 7.26.4 crash, that'll be telling depending on the stack
https://hastebin.com/idaxoxazuf.cs
now that's surprising
now this is making more sense with daniel's mailing list post
@Saran Narayan svn up, and see if trunk does better -- or show the next backtrace ;)
also, what does 'grep THREAD path/to/build/CMakeCache.txt' have for you?
okay, I think I see what's going on here... the Tcl interp is being shared across multiple threads simultaneously and it shouldn't be called at all. the problem is ged_gqa() is kicking off threads that bu_log(), which calls a hook mged registered to display everything in the command window. ged_gqa() needs to disable that hook before printing.
and I'm betting you have TCL_THREAD=1 ...
I bet that explains why it's happened to work thus far
@Saran Narayan svn up, and see if trunk does better -- or show the next backtrace ;)
https://hastebin.com/ekobopotih.cs
also, what does 'grep THREAD path/to/build/CMakeCache.txt' have for you?
https://hastebin.com/raw/akiqawiman
and I'm betting you have TCL_THREAD=1 ...
yep it is set to TRUE
a hook mged registered to display everything in the command window. ged_gqa() needs to disable that hook before printing.
yeah the hook output_callback
. I remember disabling the hook via MGED by running output_hook
command (with no arguments it disables the hook ).
I compared the performance of check_overlaps vs rtcheck. Both gave me the same timings. Around 38s in overlaps_tool and check.sh.
So I was going through gqa.c, if I had to move some parts like plane_worker, hit, and overlap to libanalyze I would need to have a huge parameter list. Some of these are from that state structure inside gqa, I planned to pass the needed variables only from the structure.
There are variables like plot_gaps, plot_overlaps... which are file pointers and they also have a list variable and a color variable. I thought of creating functions that deals with gaps, overlaps, air etc and pass a table of function pointers that act as callback functions.
and I'm betting you have TCL_THREAD=1 ...
yep it is set to TRUE
So @Saran Narayan this is definitely a difference that might cause the crash -- can you post the entire output from a clean cmake as well as your CMakeOutput.log file (in the build dir CMakeFiles dir)
So @Saran Narayan this is definitely a difference that might cause the crash -- can you post the entire output from a clean cmake as well as your CMakeOutput.log file (in the build dir CMakeFiles dir)
@Sean clean cmake : https://hastebin.com/raw/pavofuvafu and CMakeOutput.log
Mis-posted, thank you for the log!
When moving gqa to libanalyze, it would be like having different libanalyze functions for each analysis option right?
like analyze_~ where it could be analyze_gaps, analyze_volume, etc. This way we could have different check_~ files like check_gaps, check_volume etc that would call these libanalyze functions.
Was something like in your mind? This would mean some code duplication for each individual sub-commands.
The other way is to have one common driver - check. Which would call the libanalyze functions as per the choice of the user. This reduces the duplication of code.
But both approaches would make these operations exclusive like only one operation can be done at one i.e either check for overlaps or check for gaps..
Once I get an opinion about the above doubt, I can start the work.
While I was waiting, I thought I'd try this out: check_overlaps with three grid like gqa. Screenshot-from-2018-07-13-21-45-27.png
here is the diff : https://hastebin.com/lezowobevo
When moving gqa to libanalyze, it would be like having different libanalyze functions for each analysis option right?
like analyze_~ where it could be analyze_gaps, analyze_volume, etc. This way we could have different check_~ files like check_gaps, check_volume etc that would call these libanalyze functions.
That's certainly one way to handle it, not the only way.
Was something like in your mind? This would mean some code duplication for each individual sub-commands.
Sort of, but not with the code duplication! And ideally not with execution duplication either, which implies a need for separating what sets up or shoots rays from what analyzes the partitions.
The other way is to have one common driver - check. Which would call the libanalyze functions as per the choice of the user. This reduces the duplication of code.
Not sure if you're referring to command or function -- it is the intention that there will be just one command for users to call (with subcommands or options).
Once I get an opinion about the above doubt, I can start the work.
FYI, that's an Indian-English colloquialism, "doubt != questions" for everyone else. :)
While I was waiting, I thought I'd try this out: check_overlaps with three grid like gqa. Screenshot-from-2018-07-13-21-45-27.png
here is the diff : https://hastebin.com/lezowobevo
Output looks great, but too much code change to follow what's going on. Can you summarize?
The other way is to have one common driver - check. Which would call the libanalyze functions as per the choice of the user. This reduces the duplication of code.
Not sure if you're referring to command or function -- it is the intention that there will be just one command for users to call (with subcommands or options).
well I meant like having one file that would handle the options and sub-commands and setup everything for each sub-command and call the libanalyze functions for the respective sub-commands.
and each sub-command will have a callback function which will analyze the partitions.
This way setting up code would remain in one place and would be common. (which is easier said than done :D though )
we also need to talk about API design at some point since just adding more and more public functions into analyze.h isn't going to make for a good API ... it needs to be designed, needs to be simple, needs to be consistent, etc
well I meant like having one file that would handle the options and sub-commands and setup everything for each sub-command and call the libanalyze functions for the respective sub-commands.
and each sub-command will have a callback function which will analyze the partitions.
This way setting up code would remain in one place and would be common. (which is easier said than done :D though )
That sounds about like how I would probably go about implementing it, and even putting each subcommand into separate files (probably all files in a subdirectory since they're 'related').
did you come up with i,u,v or was that in the old code?
are those principle axes?
While I was waiting, I thought I'd try this out: check_overlaps with three grid like gqa. Screenshot-from-2018-07-13-21-45-27.png
here is the diff : https://hastebin.com/lezowobevoOutput looks great, but too much code change to follow what's going on. Can you summarize?
to summarize it:
did you come up with i,u,v or was that in the old code?
it was used like that in gqa. I just adapted it the struct.
ah, I see .. uv and invariant
weird
That sounds about like how I would probably go about implementing it, and even putting each subcommand into separate files (probably all files in a subdirectory since they're 'related').
okay great. The sub-directory sounds good.
just a side note -- there is behavior in gqa that we shouldn't propagate. currently in gqa, if you specify 50mm,2mm, it will stop at the level where overlaps are found. it should progress through the range specified imho.
alright got it.
so @Saran Narayan do you have time to talk about the API design?
yep
great
so you mentioned having libanalyze functions for respective sub-commands and callback functions to analyze partitions
what were you thinking there, like analyze_overlaps(), analyze_gaps(), analyze_mass(), analyze_volume(), etc?
yeah, having individual analyze_~ functions and the callback functions would be called in hit routine like gqa does with analysis flags.
but we don't have to pass analysis flags since each sub-command will have its own hit routine
you lost me a bit -- trying to figure out what code bits would be with the command (in libged) and what bits would live with the analysis function (in libanalyze) ...
basically what library is doing what -- libged should be handling the user side of things. parsing the command, subcommand, and options -- the definition of what work there is to be performed, yes?
basically what library is doing what -- libged should be handling the user side of things. parsing the command, subcommand, and options -- the definition of what work there is to be performed, yes?
yeah and libged would also do the job of analyzing the partitions and store the results
so the only question is whether it (libged) should be where knowledge of the evaluation method lives or whether libanalyze needs that
libanalyze would get function pointer and some context to this function that will evaluate
in an abstract sense, I could imagine there being a high level API like: int analyze_volume(&volume, geometry, sample_method, evaluation_method);
sample_method would be how to test the geometry like 1grid or 3grid or spherical
there would be some corresponding evaluation_method() that takes results from that sampling method, maybe even calls the sampling method internally
OR ... the concepts could be completely abstracted, in line with the "next ray" line of thinking you and daniel had talked about
yeah that is somewhat I meant too like : analyze_volume (rtip, npsw, &context, evaluation_function, grid/ray_function)
rtip would assume we're doing ray tracing
oh yeah but we need some other methods that don't do raytracing right
there are non-ray tracing methods we might be using, which is why I generalized to sample+evaluation .. not grid/ray
what's context?
the data similar to volume.. to pass to the evaluation method
don't understand -- you're already passing an evaluation_function
where's the actual volume results?
stored in context is what that looks like
typically if a function is going to have a context parameter, that's your inputs
sometimes it's inputs+outputs, but less common
int analyze_volume(&volumes, context);
then there'd be functions that work on context like analyze_context_npsw(&context, npsw); or analyze context_evaluation(&context, callback);
don't understand -- you're already passing an evaluation_function
like we have for analyze_overlaps.. we are passing the context for the evalution method ( overlap_handler) which checks for the overlaps and inserts this to the context which contains the overlap list.
int analyze_volume(&volumes, context);
then there'd be functions that work on context like analyze_context_npsw(&context, npsw); or analyze context_evaluation(&context, callback);
didn't get this
what exactly is &volumes in your example?
that would be the volume(s) that were calculated for some specified set of geometry (which would be in the context)
yeah that is what I meant by context in a general sense a structure(which would defined in libged) because there are cases where we need to pass more than one variables like overlaps_list and the plot file. Having global variables for evaluation method to access them would not be good.
if it's being passed to libanalyze, it'd need to be a libanalyze struct
oh but we won't be dealing with the struct in libanalyze.. The evaluation method defined in libged would deal with the struct.
this is why design discussion is important -- there's where the code is, where it's called from, and what the API looks like
I was specifically referring to API, not necessarily where the code is
it's useful to think about design in two modes -- one from the perspective of an end-user, another from the perspective of an application developer (API)
in this context, both of those are libged issues -- end-user is the ged command, its options
the implementation of a ged command here is calling functions, for example the current ged_gqa() calls LIBRT API almost exclusively.
we're talking about abstracting the essence, the computation of ged_gqa() into generalized API itself -- into LIBANALYZE API (almost exclusively)
so the question becomes ... what's that API?
code can/will obviously still live on the ged side of things for some parts and not for others, the question is where to draw the line
another way to think about it -- you should be able to write a header without any implementation code in libged or in libanalyze, that clearly shows how it could be used
like the analyze_volumes() function we were talking about but with all real types, actual usage
just without any implementation
you think you could try to do that next? I think it'll at least give me a better understanding of what you're thinking. could make one header for what lives in libanalyze and a separate header for what lives in libged if there is "private API" like structures or callbacks that it will know about (different from libanalyze structs/functions).
alright I can try that..
there are non-ray tracing methods we might be using, which is why I generalized to sample+evaluation .. not grid/ray
I had one question, what did you meant here with sample+evaluation. How would the sample method look like. And what would it deal with?
because right now I can only think raytracing. How can it be generalized so that we can accommodate non-rt methods.
that's getting a bit advanced, but consider one of the analysis methods like analyze_mass()
I can shoot a bunch of rays, get a volume estimate, multiply times density, and get mass
okay
or maybe I already know exactly what it is because it's a box
so sample method might be null, and evaluation might be LengthWidthHeight*Density
alright got it
oversimplified, but there are discrete methods for some things like volume -- if out geometry is a polygonal mesh, for example, I don't really need to shoot rays. you can get an exact volume value by accumulating tetrahedral volumes
yeah that is right
another example, with overlaps, I could calculate the intersection of all surfaces to get the "overlapping" regions as geometry, then any areas with non-zero volume would be my overlaps
there, the sample method would be to calculate intersecting surfaces, the evaluation method would probably figure out the resulting volumes
not a single ray is needed, the result is more precise
that all said..... our current methods DO use ray tracing and that's certainly important to consider
we do need to shoot 1-grid and 3-grids
so on the surface, just moving everything in ged_gqa from libged to libanalyze would be a step in the right direction
aah got it now.
the problem is eliminating the duplication with the 1-grid method and not exploding libanalyze API in the process
right now (and this is no fault of yours) libanalyze is a complete mess, it's just a dumping ground of structures and functions
completely useless as an API
at least as a PUBLIC api
pretty much everything in there should be private API, private to the implementation
okay got it.
so forgetting about everything you know about the current check_overlaps, rtcheck, gqa, etc ... if you wanted to have a function that calculated volume, the question I have is what that should look like as public API (public to a library), so that another application or user could call that function and get a volume as easy and simple as possible
at heart without knowing anything about sample methods or our existing libraries/types, it might look something like: int volume(double *volume, const struct geometry *object);
unambiguous, simple, right?
yeah
without knowing anything, that looks like it's probably an error/success return code, volume is returned in 'volume' and I probably pass geometry as the 'object' parameter
that's close to ideal
so then the reason it can't be that simple ... getting volume can be really hard for some geometry types
yeah
so at a minimum, we might think to introduce a quality, precision, or accuracy control
maybe something like: int volume(double *volume, const struct geometry *object, int quality) where quality=0 is fast as possible and quality=100 is exact as possible
yeah that still is very abstract
YES!
that quality parameter is also very vague and maybe quality=100 is impossible to be exact
having it be arbitrary depending on the geometry wouldn't be good, so maybe we change it to within a specified tolerance
int volume(double *v, const struct geometry *g, double tolerance);
yep this makes more sense
yep, EXCEPT, we still cannot really guarantee a particular tolerance
consider what rtcheck does with a grid of rays method
exactly..
rays are infinitely thin
yup
so I might shoot a grid of rays at a lattice...
every ray might hit
shift the grid and every ray might miss
double the rays and every ray might hit again, double again and they might all hit again
double again and they might all miss
I can't say much about tolerance because it's not the spacing between the grid cells
so we need to rethink the API .. quality is no good, tolerance is not possible, what else might we do?
there is a caveat on tolerance -- it may actually be possible using monte carlo methods, but I'm not sure we could prove it
so we need to rethink the API .. quality is no good, tolerance is not possible, what else might we do?
alright what can we use then? that is still abstract
not just abstract, it's high-level -- trying to be as high level as possible since that will result in the easiest use. I mean, without knowing anything, it'd be easy to use this function in an int main(int ac, char *av[]) { ... get geometry from file ... ; double v; volume(&v, geometry); printf("volume is %lf\n", v); return 0; }
so that tolerance that couldn't work was a distance tolerance ... a different kind of tolerance might work which would be a calculation tolerance of sorts
e.g., int volume(double *v, const struct geometry *g, int significant_digits);
how would that work? Calculate volume accurately until we satisfy the significant digits condition?
yep, iterate until the numbers seem stable to that level
could also be "significant digits after the decimal point" if you wanted to make it size-agnostic
so in case of rtcheck what could be the iteration condition? making the grid finer?
yep
lots of possible strategies, but they're all variations on shooting finer grids until the numbers seem to agree
hmm all of these are like examples of API in general. Would MGED require such high-levelness ? The end-user won't be writing any code, they would only launch commands like check volume.
end user of library is a developer
yeah that is right
so does a dev require it -- of course not, but that's like saying a user doesn't need "check volume" to be simple and easy to use ... of course it can be more complicated, but it shouldn't if it's done right ;)
it also doesn't mean there's not a lot of nasty details underneath in private API
that said, we're still not done... we just found what might be reasonable termination criteria with significant digits, but now there's a practical problem
we know the methods we know we'll have to use to get significant_digits=10 to complete ... is going to be expensive
time consuming
yeah indeed
a call to volume(&v, &g, 10) might take 10 minutes or 10 days
haha true
or might take 0.003 seconds
it depends on &g
so what do we do?
decide the methods that are less expensive as per the object
say the method is fixed, like rtcheck
we don't have a method parameter -- so that could be a solution -- let the caller specify a method, but it still could take seconds or days so we need something more
humm what could that be
not necessarily the best idea, but can always ask the caller
if time is a problem, could have a time limit parameter
anything else? are there maybe ways time could be reduced?
sampling methods?
what do you mean?
say the method is fixed -- it's rtcheck
rtcheck-style rather -- it's a grid
well the way rtcheck shoots the grids.. in chunks , up down etc
that is a bit complicated for the end-user to specify tho
not necessarily, an "enum type" integer could do it
good thinking, but that's still not going to really change the time
going multithreaded could definitely change the time
but how many CPUs the user wants us to utilize then becomes an issue since they may not want us using all of them (or it may even cause their machine to crash)
hence parameter like ...
number of processors to use
bingo
but now it just got a little weird: int volume(double *v, const geometry *g, int sig_dig, int npsw)
npsw has nothing to do with 'volume' and entirely to do with the implementation
that's where most APIs introduce the concept of a context or attributes or parameters or ... lots of different names for ways to handle all the inputs together so you don't end up adding additional parameters to your function
because npsw might just be #4, then someone else figures out they need one more and then another and before too long you have 20 arguments and an unusable API with too many knobs
so we can group those into something a little more general and optionally manageable
aah like additional parameters
maybe: int volume(double *v, const struct context *c);
humm and these additional variables would need meaningful defaults too
but then we need functions to set that context up like add_geometry(struct context *c); set_significant_digits(struct context *c); set_npsw(struct context *c);
ohh right now I am understanding what you meant
and could -- if they had meaningful defaults like npsw=ncpu then maybe we wouldn't even call set_npsw unless the user wanted it lower
yeah
so we still have a problem though
even multithreaded, it could take seconds or days .. so that didn't really solve the original problem
yeah..
it just greatly decreased the chance performance will be a problem
and gave the caller a necessary control
so what's left? what are our options?
to give control over methods ? :grimacing:
that's definitely an option
make the interface less simple by exposing more control over the implementation
oh like the options ? grid spacing etc?
that's kind of where we started this conversation, what was it .. something like volume(struct volumes*v, func_t sample_method, func_t evaluation_method)
which implied a couple more function arguments to specify their inputs
or grouping them like our context concept, where you'd set the function pointers on the context
volume(struct volumes *v, struct sample_context *c, struct eval_context *e);
does that make more sense now?
oh okay.. yeah now I understand the concept of context..
but I have this question.. different sampling methods/ evaluation methods would have different signatures
so that would imply things like set_geometry(struct sample_context *c, struct geometry *g); and set_significant_digits(struct eval_context *e, int digits);
yes they probably would .. maybe .. maybe not :)
yeah.. having same signatures would be good.. but challenging..
so that would imply things like set_geometry(struct sample_context *c, struct geometry *g); and set_significant_digits(struct eval_context *e, int digits);
right got it
so these structs of context would be defined in libanalyze as you had mentioned before.. now its coming together
yay
but we are not exposing it to the user.. but abstracting it with some functions to set it up
you don't have to call them that if you know of a better word, but that's a common term for grouping inputs together
technically, there's no reason for having two, and less is more
yeah that is right.. we can totally do it with just one context
but more is easier to do :D
volume(struct volumes *v, struct context *c); ... set_sample_method(struct context c, int (sample_method)(...), struct inputs *data); set_evaluation_method(struct context c, int (eval_method)(), struct inputs *data); .. etc
not sure it's actually easier -- the reason for doing two would be if there's some strong compelling reason to keep them separated, like if there was an incompatibility with other related functions
in this instance, I think it actually strongly warrants having just one context because the sample method and eval method are highly related to each other (their implementation will almost certainly be in pairs)
yeah that is true
so you think this is a header you could try writing, something that just covers the scope of rtcheck and gqa?
there's a lot of detail that wasn't written down, probably some mistakes in assumptions
spending just a couple hours thinking on this would be beneficial even if you don't end up using it when you get back to the immediate issue at hand, which is moving gqa code to libanalyze
yeah I will think about it..
about gqa, there is one challenge I faced with check_overlaps today. I was hoping to have a common grid_setup for single and 3 -grid but gqa doesn't support az/el so I had to make it separate..
And I thought about what would az/el support look like in gqa.. Could not think of anything
detail I didn't mention is how you specify all the options for a given sample method like rtcheck's parameters -- that's what something like that "input" structure could hold -- a simple dictionary of key=values that the sample/evaluation method could pull from if they're set
gqa supporting az el has been a request for years :)
if you can do grid setup for 1-grid, then you should be able to do 3-grid...
gqa's 3-grid is just az/el 0,0, and 0,90 and 90,0
or something close to that, maybe -90
but both are different.. one forms grid around the bound_box while, rtcheck forms a gird around the eye
yes, but the grid around the bound_box is the same as a grid around three different eye positions...
what's different is gqa's clever incremental stepping so it doesn't reshoot the same rays
yes, but the grid around the bound_box is the same as a grid around three different eye positions...
hmm if that is the case then it shouldn't be difficult. I just could not visualize it :D
well I wouldn't sweat it -- if it's not easy, then keep them separate .. I'll just be sad about it ;(
what's different is gqa's clever incremental stepping so it doesn't reshoot the same rays
yeah, I did with skipping one ray for every even row
at some point in the next week or so, you need to prove that it's right, that it shoots the same grid incrementally as gqa
not just check the overlap list, but show the rays like 10x10x10, then 20x20x20
yeah got it ;)
and oh one more thing.. rtcheck supports uneven grid shape.. like in rectangles but gqa shoots in square girds..
like with -g10 -G20 I am shooting 10mm by 20mm grids
rectangular grids is not important
that's an artifact of it being an rt* application
humm understood.. it has to do with the image width and height right
related
it has to do with the size and shape of a "pixel" possibly being not square, depending on an output device
yeah that makes sense!
if you're displaying on a TV, for example at 4:3 aspect ratio, you might want non-square pixels to get the best image
aah right
that is entirely counterproductive for a sampling algorithm that is just trying to find overlaps
ohk so square grids it is then :)
alright then gonna grab some sleep now.. Thank you for spending the time explaining the API in such detail :)
ah right, what time is it there?
its around 1 AM :D
you can check local time by clicking on the user ;)
ah, cool. well thanks for staying up late to talk through this -- it helps me sort thoughts to talk through things too :)
ah, so I can, neat
/me still prefers /ctcp
cool, they added /me support
/ctcp ping Sean
but not that :)
so you think this is a header you could try writing, something that just covers the scope of rtcheck and gqa?
My attempt at it.. https://hastebin.com/topudekoza.cs
I couldn't think up any evaluation methods. When the user issues any command like analyze_overlaps then default evaluation method would be used
hmm the analyze_volume and analyze_weights need some sort of list for different objects mentioned by the user only double won't work
BTW @Sean What does the src/libanalyze/api.c do? it looks very similar to gqa
This is what had planned and have been working on the last few days.
in libged/check dir:
in libanalyze : check_analyze_geometry.c
To summarize,
In libged/check/check.c -
In check/check_overlaps.c -
In libanalyze/check_geometry.c
Similarly for other functions, like check_volume, there would be a hit function, so a function pointer to the volume specific hit function is passed to libanalyze. Also a volume specific callback data would also be passed to libanalyze. The flow would go to libged when there is a hit.
I have only done the check_overlaps. The others are left. Before proceeding I needed an opinion on this idea..
PS: the files may not be proper right now, would review it before committing of course. I would commit only if this idea is fine :)
How does it work, how is your experience with it? Does 'check overlaps ...' work? Does it simplify the things? check_overlaps.c is much smaller than before (which is good), do you expect it to be similar for the other commands?
yep the command check overlaps is working fine.
Is it simpler? Well it is the same as check_overlaps other than you have to use -N 1 for single grid. Be default it behaves as gqa. I liked the option to refine the grid. Something that was missing in check_overlaps/rtcheck. Plus there is now option to output plot file and also display them.
About the other commands, it may be a bit complicated, like the code is right now ready to work with 3 grids, would need to adapt it for single grid too.
What concerns me is that it does not actually behave as the API design me and sean discussed. The whole code for gqa does not live in libanalyze. So something like analyze_volume(&volumes, context) is not gonna work
oh one thing different from check_overlaps is that when refining the grid, the overlaps are not counted twice. I think it is because we are passing an a_overlap function which disregards the partition once its seen by returning a 1. a_logoverlap would duplicate record the overlaps on refining.
What concerns me is that it does not actually behave as the API design me and sean discussed. The whole code for gqa does not live in libanalyze. So something like analyze_volume(&volumes, context) is not gonna work
this is done with libanalyze/api.c, I saw the commit history and patch on sourceforge it was originally done to work with ged_analyze. Maybe I can fix it up, remove the globals and add analyze_~ functions for what is missing?
What concerns me is that it does not actually behave as the API design me and sean discussed. The whole code for gqa does not live in libanalyze. So something like analyze_volume(&volumes, context) is not gonna work
this is done with libanalyze/api.c, I saw the commit history and patch on sourceforge it was originally done to work with ged_analyze. Maybe I can fix it up, remove the globals and add analyze_~ functions for what is missing?
Maybe ;)
About the other commands, it may be a bit complicated, like the code is right now ready to work with 3 grids, would need to adapt it for single grid too.
What#s wrong with computing the volume with three grids? It could give better results, or? Like: (grid(x) + grid(y) + grid(z)) / 3
?
About the other commands, it may be a bit complicated, like the code is right now ready to work with 3 grids, would need to adapt it for single grid too.
What#s wrong with computing the volume with three grids? It could give better results, or? Like:
(grid(x) + grid(y) + grid(z)) / 3
?
I meant like if I make gqa behave like rtcheck with single grid some calculations need changing and some might not even work.
hmm there is one disadvantage of moving from gqa to check. Because check works as subcommands. I lose the feature of doing many analysis like -Avo for getting volume as well as overlaps in gqa.
I meant like if I make gqa behave like rtcheck with single grid some calculations need changing and some might not even work.
Please correct me if I'm wrong but, I thought that gqa works with 3 grids and rtcheck with one, and you are trying to do rtcheck with 3 grids?
Or, 'check overlaps' with 3 grids?
hmm the plan was to merge these commands(gqa, rtcheck and glint) into one command check
hmm there is one disadvantage of moving from gqa to check. Because check works as subcommands. I lose the feature of doing many analysis like -Avo for getting volume as well as overlaps in gqa.
This was my consideration too.
so something like check overlaps -g50,10 -a35 -e25 -N1 g4 would behave as rtcheck/check_overlaps and something like check overlaps -g50,10 g4 would behave as gqa
hmm the plan was to merge these commands(gqa, rtcheck and glint) into one command check
Yes, something like taking the best of all worlds ;)
yeah :)
so somethings like volume or weight is best done with triple grid
so something like check overlaps -g50,10 -a35 -e25 -N1 g4 would behave as rtcheck/check_overlaps and something like check overlaps -g50,10 g4 would behave as gqa
Maybe, if somebody wants to know only the overlaps the 3 grids are taken, but if the grid is explicitly declared on the command line with azimuth, elevation, etc., then only this single grid will be used.
yeah that would be better! right now I made it like if the user mentions az/el and does not pass -N1 then a message appears that az/el not implemented for triple grids.
ohh this is how I added triple grids: https://hastebin.com/ruxejuxoco
not sure if you'd approve of those extra vars.
if I were to move gqa completely to libanalyze, then this is my plan.
for check command. On libged side:
The advantage of having something like this would be like if in future there is a need of volume in any other command then the programmer can simply prepare the ray-tracing context and call analyze_volume to get the result easily.
One can also do multi-analysis just have to pass the flags to analyze_raytracing_context_init (...) and call the respective analyze_~ commands.
not sure if you'd approve of those extra vars.
Well, if there are necessary to control the 3 grids. BTW, the initialization of max_views is in another file?
not sure if you'd approve of those extra vars.
Well, if there are necessary to control the 3 grids. BTW, the initialization of max_views is in another file?
yeah, done in libged for now. But was thinking to move it to libanalyze according to the above message.
one thing that would be different would be those commands that deal with lists like overlaps, gaps, adj_air, exp_air...
Because these lists are not produced when the user requests like analyze_overlaps(&context,..) instead they are created when ray-tracing.
One could pass a pointer to a local list pointer but then these structure should declared in analyze.h so that it can be accessible by both libged and libanalyze.
What do you say about the plan? or should I just stick with current plan?
The new plan would need lots of public functions defined in include/analyze.h
I thought there was the wish to move the functionality to general purpose functions in libanalyse, as you described it here. The user visible interfaces and programs (gqa, rtcheck, ...) should stay the same, at least for the moment, but their backend can be moved to libanalyze. This way they can be used in other places too. And to get the full advantage of the new functions there are the new libged check
commands.
To do this you had to implement the new libanalyze functions first and the libged check commands next.
Sounds like a plan.
(?)
Regarding the lists: Aren't all lists generated on the user's side when it provides the call-back?
I thought there was the wish to move the functionality to general purpose functions in libanalyse, as you described it here. The user visible interfaces and programs (gqa, rtcheck, ...) should stay the same, at least for the moment, but their backend can be moved to libanalyze.
hmm so you are suggesting that the commands still remain the same but their back-end changes. Like we would be using the new API in the exisiting functions like ged_gqa and ged_rtcheck etc..
Regarding the lists: Aren't all lists generated on the user's side when it provides the call-back?
yeah that could work also.. but commands like adj_air, gaps, .. are identified in the hit function so I got confused.
but just calling the callback function with callback data is good
I thought there was the wish to move the functionality to general purpose functions in libanalyse, as you described it here. The user visible interfaces and programs (gqa, rtcheck, ...) should stay the same, at least for the moment, but their backend can be moved to libanalyze.
hmm so you are suggesting that the commands still remain the same but their back-end changes. Like we would be using the new API in the exisiting functions like ged_gqa and ged_rtcheck etc..
oh wait. Did I get this wrong? Did you mean keeping the gqa, rtcheck, etc as it is for now and just make a copy of the backend in libanalyze. I thought when u said move, you meant literally how move works, like cut paste xD
And use this backend in the new check commands.
As Sean mentioned some time ago: User visible changes in the sense of a command not be working any more as expected need to be announced. See the CHANGES file for our policy. This is for users which have scripts using rtcheck or gqa for example. These shall not break without warning.
oh wait. Did I get this wrong? Did you mean keeping the gqa, rtcheck, etc as it is for now and just make a copy of the backend in libanalyze. I thought when u said move, you meant literally how move works, like cut paste xD
And use this backend in the new check commands.
Yes, the first step is the copy. Then the implementation of check ...
. The last step would be the rewrite of rtcheck (alredy done?), gqa, etc. with the new backend.
oh wait. Did I get this wrong? Did you mean keeping the gqa, rtcheck, etc as it is for now and just make a copy of the backend in libanalyze. I thought when u said move, you meant literally how move works, like cut paste xD
And use this backend in the new check commands.Yes, the first step is the copy. Then the implementation of
check ...
. The last step would be the rewrite of rtcheck (alredy done?), gqa, etc. with the new backend.
okay got it. and well rtcheck would have to be re-written because this would be different. Since the one I had done uses analyze_overlaps. Which is totally different than the general API
or should I keep analyze_overlaps as it is.. because I can just pass a triple_grid_generator and have a gqa equivalent functionality from it
check_overlaps with three grid like gqa. Screenshot-from-2018-07-13-21-45-27.png
I tried it here
or should I keep analyze_overlaps as it is.. because I can just pass a triple_grid_generator and have a gqa equivalent functionality from it
Well, it would provide a redundant functionality, wouldn't it? This isn't optimal.
However, going this way you would make sure that you don't break anything. The plan is very ambitious, therefore it is good to have an exit strategy.
However, going this way you would make sure that you don't break anything. The plan is very ambitious, therefore it is good to have an exit strategy.
yeah. That sounds good.
Another idea: Having this new check-API in libanalyse, could you think of having convenience functions in libanalyse on top of them which ease the use of them for simple tasks as retrieving the volume?
oh like a function that calls these check-API functions privately and provide an easy public API?
I have check overlaps and check volume working now.
I followed sean's suggestion and used key=value pair for passing options from libged to libanalyze using bu_hash_tbl.
check_options.c
This I can have meaningful defaults on libanalyze and just set those values which are passed from libged
Here are my libged/check/ files
check.c - main driver
check_private.h
check_volume.c - check_volume() lives here
check_overlaps.c- check_overlaps() lives here
This I can have meaningful defaults on libanalyze and just set those values which are passed from libged
This is how I am parsing the options in libanalyze/api.c : https://hastebin.com/rowerukure.cpp
I plan to pass the callback function and callback data combo for the analysis options that deals with lists. So I would have callback function for gaps, adj_air and exposed_air
I was also wondering about the options that deal with debug flag, verbose flag, quiet_missed_report_flag and print_per_region_stats flags.
Pass a bu_vls struct for printing purposes so libged would print everything at the end? Because bu_log would mean crashes in multi-threaded places of code.
I have check overlaps and check volume working now.
Screenshot-from-2018-07-21-22-20-01.png
You solution with separate context-table and current_state structures is unusual, but probably reasonable. Another possible solution would have been to use state setting functions as e.g. set_state_azimuth(struct current_state *state, fastf_t azimuth)
in libged.
I was also wondering about the options that deal with debug flag, verbose flag, quiet_missed_report_flag and print_per_region_stats flags.
Pass a bu_vls struct for printing purposes so libged would print everything at the end? Because bu_log would mean crashes in multi-threaded places of code.
Doing the printing at the end in the main thread should surely solve the multi-threaded Tk issue.
You solution with separate context-table and current_state structures is unusual, but probably reasonable. Another possible solution would have been to use state setting functions as e.g.
set_state_azimuth(struct current_state *state, fastf_t azimuth)
in libged.
hmm but having the current state struct in analyze.h seemed not so good. Like it contains the internal matters.
But storing the parameters or options in ray _tracing_context is feasible.
That would require a init function that would set the defaults.
I did separately in the sense if we add non-raytracing functions in the future there would be one common options struct to get options from libged to libanalyze.
I was also wondering about the options that deal with debug flag, verbose flag, quiet_missed_report_flag and print_per_region_stats flags.
Pass a bu_vls struct for printing purposes so libged would print everything at the end? Because bu_log would mean crashes in multi-threaded places of code.Doing the printing at the end in the main thread should surely solve the multi-threaded Tk issue.
Not sure if I follow. Are you suggesting to stick to bu_log in libanalyze but print these information in main thread?
hmm but having the current state struct in analyze.h seemed not so good. Like it contains the internal matters.
You don't need to publish the content of current_state. A simple forward declaration (struct current_state;
) in the public header is sufficient.
But storing the parameters or options in ray _tracing_context is feasible.
That would require a init function that would set the defaults.
That's true, but you need this for the table as well.
However, don't misinterpret my comments here. Your solution isn't wrong or bad. I want you to learn about alternatives. We learn for live ;)
I was also wondering about the options that deal with debug flag, verbose flag, quiet_missed_report_flag and print_per_region_stats flags.
Pass a bu_vls struct for printing purposes so libged would print everything at the end? Because bu_log would mean crashes in multi-threaded places of code.Doing the printing at the end in the main thread should surely solve the multi-threaded Tk issue.
Not sure if I follow. Are you suggesting to stick to bu_log in libanalyze but print these information in main thread?
Not sure if I follow ;)
I was simply following your argumentation to "print" the log messages to a bu_vls structure and doing the output when the processing is back in the main thread in libged(?)
hmm but having the current state struct in analyze.h seemed not so good. Like it contains the internal matters.
You don't need to publish the content of current_state. A simple forward declaration (
struct current_state;
) in the public header is sufficient.But storing the parameters or options in ray _tracing_context is feasible.
That would require a init function that would set the defaults.That's true, but you need this for the table as well.
Alright that sounds good then. Will give it a try!
This way seems better and would be less complex IMO ;)
However, don't misinterpret my comments here. Your solution isn't wrong or bad. I want you to learn about alternatives. We learn for live ;)
Definitely. I like exploring the alternatives.
okay that is done :)
Will review the code and push it by tomo
I added check gap and check exp_air today. Everything done so far is committed to the repo.
Added adj_air now
Added check weight
and analyze_weight function
@Daniel Rossberg there is this default density flag in libanalyze/api.c. Which treats all materials as Aluminum, 7079-T6 with density as 2.74.
Is this a desired feature?
@Daniel Rossberg there is this default density flag in libanalyze/api.c. Which treats all materials as Aluminum, 7079-T6 with density as 2.74.
Is this a desired feature?
If you request a weight, you need to provide a density. In this sense any positive value could be used as default. The 7079-T6 aluminium is however already known in BRL-CAD (grep for 7079-T6).
@Daniel Rossberg there is this default density flag in libanalyze/api.c. Which treats all materials as Aluminum, 7079-T6 with density as 2.74.
Is this a desired feature?If you request a weight, you need to provide a density. In this sense any positive value could be used as default. The 7079-T6 aluminium is however already known in BRL-CAD (grep for 7079-T6).
okay, but gqa doesn't set any default densities. If the density for the material ID is not found it aborts the ray-tracing. However api.c runs it. Shows some BAD LOS erro messages and calculates a weight. I am not sure if the weight is right as I can't compare with gqa
I did confirm it working by making a region with one cube and assigning it a material ID of 2 (copper tool steel) and compared the results of gqa and check weight
I passed the density file as the one found in man page of gqa
That's a hack (imho) because of rtcheck/gqa usability failings.
Someone was trying to make it is to just a tool work without specifying a density table and almost certainly thought that it's more likely density-wise that the object is made of aluminum (as opposed to picking some other value like water==1.0). The real issue is that it's clunky to specify densities for geometry. Rtcheck demands a text file with a specific syntax. GQA demands this as well, and then demands that you suck it into your .g file where it is no longer as easily edited and must be re-imported repeatedly if it changes.
There is otherwise no special purpose for 7079-T6 that I'm aware of. It's just assuming the object is most-likely a vehicle and most vehicles are mostly aluminum. I don't know of anyone that uses that default for anything, don't believe it's documented which means it can change at will.
Someone was trying to make it is to just a tool work without specifying a density table and almost certainly thought that it's more likely density-wise that the object is made of aluminum (as opposed to picking some other value like water==1.0). The real issue is that it's clunky to specify densities for geometry. Rtcheck demands a text file with a specific syntax. GQA demands this as well, and then demands that you suck it into your .g file where it is no longer as easily edited and must be re-imported repeatedly if it changes.
yeah that is right. BTW Is there like a common database for these densities for the materials used in BRL-CAD? or something that matches the material ID to materials?
misc/GQA_SAMPLE_DENSITIES
hmm well that is the same as the one found in man page of gqa :D
I was asking because gqa was complaining about material IDs not found. So I was wondering like if users can put any random values in material ID ?
If a material ID is valid or not depends on the material database provided. If the ID can be found there it should be OK.
I added the verbose and debug printing option just now
so was thinking if we were to replace gqa's back-end
the order of the printing of this information would be different
the real tables used in practice are considerably longer and more vetted than the sample densities
that's something I'd love to get released, but then we'd really want a better way to manage it than a density text file -- proper material objects or a material database that it can pull properties from
the order of the printing of this information would be different
I had several conversations with modelers when the tcl checker.sh was written and the default encounter-ordering wasn't important -- they typically want to review and address issues in priority order, which is typically largest to smallest
@Saran Narayan one request for the new check command ... s/weight/mass/g ! it's a bit annoying that has remained for so long.
that's something I'd love to get released, but then we'd really want a better way to manage it than a density text file -- proper material objects or a material database that it can pull properties from
maybe like have a database of well-know materials and their densities from which a user can choose from when creating regions.
And if the user wants to add any new material then they must also provide the density which gets copied to that .g file.
So when analysis tools run, they would know where to pull this information from.
exactly
@Saran Narayan one request for the new check command ... s/weight/mass/g ! it's a bit annoying that has remained for so long.
sorry I didn't get you. I am not familiar with s/word/word/g format
substitute weight for mass everywhere ... we don't actually calculate weight
okay :)
So I was talking about the debug information. There is also one thing other than ordering
the debug flag also prints the ray_dir and ray_pt
but I am using the rectangular_grid function.
oh wait never mind :D I can print that just before shooting
debug is for devs, can be whatever we need it to be
okay got it
frankly, debugging could disappear so long as we have a way to know with certainty that there's no bugs (which is unlikely)
more important is that it works in parallel, that it shoots the same rays, that -- barring bugs in the original -- it gives the same result set (regardless of ordering)
hmm okay
@Saran Narayan on that note -- I think your build logs indicated the TCL_THREADS was on for you, yes? Would you do a build test where you force it off -- see if gqa still has a problem?
was talking about this with @starseeker and he was a bit surprised that it was on -- that may be why we don't see issues elsewhere if something is awry in the build logic.
@Saran Narayan on that note -- I think your build logs indicated the TCL_THREADS was on for you, yes? Would you do a build test where you force it off -- see if gqa still has a problem?
yup can do.
that worked. now it does not crash.
anyway I noticed this strange behaviour:
edit: gif removed
part of the messages are printed later sometimes
meh that gif is not clear
Screenshot-from-2018-07-24-01-07-09.png
those are diagnostic messages from librt -- it's saying a cylinder was hit 3 times
would be interesting to see those specific rays visualized
yeah but since these messages are printed using bu_log, they should appear as red and above the bu_vls_printf(gedp->result_str) messages
would be interesting to see those specific rays visualized
well the struct hit contains a pointer to struct xray * so we can get information about ray from it
yeah but since these messages are printed using bu_log, they should appear as red and above the bu_vls_printf(gedp->result_str) messages
ideally, but it depends on what the state of the channels and hooks are at the time it logs
it "should" be distinguished separately ...
hmm okay then. Now that bu_log seems to be fixed for me, should I revert back to bu_log instead of passing a bu_vls struct for printing purposes in libanalyze?
bu_log() is fixed? nothing has changed that I'm aware of
so I wouldn't say "fixed" :)
I don't think calling bu_log is any more appropriate than passing a bu_vls
in fact, that style of having a logging ledger is sort of what the other ged commands need (whether going through a logging api or not)
hmm okay but one thing is good about bu_log that it displays information as it happens.. Like those grid refining messages. It's an easy way to know something is happening.
I added moments and centroid
right now working on surface area
I saw you commits, you are making really great progress. You seem to accelerate to the end :grinning:
What about your patches on sourceforge? I suppose that I can close #495?
I saw you commits, you are making really great progress. You seem to accelerate to the end :grinning:
Haha thank you :). I have accelerated because from Aug 1, I have my regular classes from 9:30 - 16:30. So my time allocated would be affected. Hence trying my best to complete as much as I can :).
What about your patches on sourceforge? I suppose that I can close #495?
hmm #495 is outdated so it can be closed
for #497 I will submit an updated one without the rtcheck related removals
#497 is probable outdated too, you redesigned the whole libged stuff, etc.. Therefore ... there you said it :)
yeah check overlaps should replace check_overlaps right?
but there is one thing I have not added from check_overlaps -- getting view information from view
when -a, -e and objs are not mentioned
it takes objects from view but not the view information
There is probable a reason for it (?)
yeah, check defaults to gqa behaviour if nothing is mentioned like check overlaps
. We can give check overlaps -a35 -e25 overlaps
for single grid.
only solution there is to add one extra option
and I could add one analyze_get_from_view(state, gedp) function. Which setups state
triggered by a special option of check overlaps
?
yup
OK :)
for surface area
since there are no units table
I am doing
bu_vls_printf(_ged_current_gedp->ged_result_str, "\t%s %g %s^2\n", tobjtab[i], surf_area / (options->units[LINE]->val*options->units[LINE]->val), options->units[LINE]->name);
or should I add one more table?
I think what you are doing looks OK.
alright cool
also should I add some kind of warning, when single grid is chosen
and user asks for mass/centroid/moments
I don't know. Personally, I wouldn't do it if the user explicitly gave the parameters for the single grid.
but the values returned are way off
Is it so bad?
https://hastebin.com/jolovakexa.css
comparison
What was it, the havoc?
nope
Screenshot-from-2018-07-24-23-58-09.png
the inner cube subtracted and created a region
What's it size?
the arb8 is 39.3701 X 39.3701 X 39.3701 and arb7 is all values are abs(157.4803)
i just created then randomly
and material Id is 2
mm?
inches
https://hastebin.com/ulefabapij.css here gqa vs check for triple grid
Have you computed the volume by hand? The difference is a little bit to big (1.63 vs. 5.43) for such a simple geometry. A factor of 3?
Have you tried it for a simple cube?
hmm I will try on something simple like the cube.. there you said it
https://www.diffchecker.com/cJNUrcoA
comparison with the simple cube of 40x40in
hmm the dimensions doesn't make sense. Let me recheck from .g
its 2000x2000x2000mm
I said 39.3701in, but it was the coordinates :sweat_smile:
The single grid is about 1/3rd of the triple grid?
how are we comparing them?
8 vs. 2.69 m^3 (?)
triple grid shoots 39x39x39 rays and single grid shoots 69x69 rays
8 vs. 2.69 m^3 (?)
yeah that seems awfully close to 1/3
I tried out gqa -Av -N1 to force single view
but that result came out to be 8 m^3
E.g. for geometries made of sheets the single grid method could be very wrong, but for a solid cube?
I didn't get what you meant by made of sheets?
I will do one thing.. draw the single grid formed with ray_pt values.
If parts of the geometry are invisible from a view point as e.g. thin metal sheets (or plates).
If parts of the geometry are invisible from a view point as e.g. thin metal sheets (or plates).
hmm then three grids are best
I will do one thing.. draw the single grid formed with ray_pt values.
and see how much is the coverage
in theory it should cover the whole geometry
What's state->num_views? Can it only be set with the -N
option?
yeah and it's set to 1 with with -ae mentioning
At which code line?
api.c line 1231
line 117 libanalyze/check_options.c
oh wait I have some changes in api.c that is yet to commit related to surface area. So line number can vary
Yes, a little bit, but I could find it.
Well, it looks like there is a factor of 3 somewhere ... or something else.
yep! here is the grid formed https://www.geogebra.org/3d/vdq9yhbq.
I will try to investigate it
one thing I noticed is it shoots one less, like in check_overlaps it shoots 70x70 grid. But check shoots 69x69 grid
Screenshot-from-2018-07-25-01-03-20.png
oh nvm this. The cube is drawn wrong xD. The centre should be 0,0,0
okay I think I figured it out! It's the difference in number of shots.
8/2.68851 = 2.975 is the factor.
40x40 is the grid formed per view in case of triple grid = 1600 shots
69x69 is the grid formed for single grid = 4761 shots
and 4761/1600 = 2.975
Not sure how to fix it easily
@Saran Narayan how much difference is there if you run 1 grid vs 3 grid but to the same grid density (e.g., in your example, looks like 3 grid went to 639^3 but only checked 1 grid to 277^2)
the arb8 is 39.3701 X 39.3701 X 39.3701 and arb7 is all values are abs(157.4803)
you can try running the 'analyze' command on the primitives -- I believe both of those should compute volume correctly (exactly), then you can do the subtraction to get the actual volume vs check/gqa's estimate
okay I think I figured it out! It's the difference in number of shots.
8/2.68851 = 2.975 is the factor.
40x40 is the grid formed per view in case of triple grid = 1600 shots
69x69 is the grid formed for single grid = 4761 shots
and 4761/1600 = 2.975
This isn't making sense to me. There should only be that big of a difference if the grid sizes where the same for both, but the 40x40 and 69x69 should be quite different sizes (per cell). E.g., if we were shooting a 1000x1000mm wide view, 40x40 grid would have 25mm cell width and 69x69 would be about 14.5mm. If half the shots hit something and reported a thickness of 500mm, for example -- the volume estimate for the 40x40 grid would be exactly 2.5e8mm^3 (because 25^2 * 500 * 40*40/2) and the estimate for 69x69 would be either 2.46e8mm^3 or 2.54e8mm^3, depending on whether we used 34 or 35 cells for "half" of 69. Either way, the answers are pretty darn close ...
hmm in the example you gave you kept the viewsize the same. But its not the case with 1 grid vs 3 grid.
For example a 2M x 2M x 2M cube, mdl_max : (1000,1000,1000) mm and mdl_min: (-1000, -1000, -1000) mm
Span is VSUB2(mdl_max, mdl_min) = (2000, 2000, 2000).
For 3 grid the viewsize is 2000mm and area is 2000x2000mm, so for 50mm gridspacing the grid formed is 40x40.
For 1 grid the viewsize is taken as MAGNITUDE(span) = 3464.101mm and area is 3464.101mm x 3464.101mm, so for 50mm gridspacing the grid formed is 69.28 x 69.28 ~ 69x69.
the formula : o_volume[view] = o_len[view] * (area[view] / shots[view])
In case of 3grid, the area of the bounding box (since its a cube, its is 2000x2000) = area of the grid so this works out.
In case of 1grid, the area of the bounding box != area of the grid as we saw.
Since the area is kept the same = 4e+06 but the number of shots vary. Because of the difference (40x40 vs 69x69).
The formula becomes:
in case of 3grid, o_volume[view] = o_len[view] * 2500
in case of 1grid, o_volume[view] = o_len[view] * 840.159 only
@Saran Narayan how much difference is there if you run 1 grid vs 3 grid but to the same grid density (e.g., in your example, looks like 3 grid went to 639^3 but only checked 1 grid to 277^2)
welp I cannot control the density of the grid, but spacing I can keep the same: https://hastebin.com/uxozakodep.css.
the arb8 is 39.3701 X 39.3701 X 39.3701 and arb7 is all values are abs(157.4803)
you can try running the 'analyze' command on the primitives -- I believe both of those should compute volume correctly (exactly), then you can do the subtraction to get the actual volume vs check/gqa's estimate
wow thanks for the tip, the analyze command is great :)
did the manual calculation : according to analyze its around 162.667 m^3 and according to check/gqa it is around 162.888 m^3.
I added this to single_grid_setup state->area[0] = state->viewsize * state->viewsize
. Now looks much better :) https://hastebin.com/raw/dizagoboxu
Enabled print per-region stats option for volume and mass
added the view information option as -i for check overlaps.
I added this to single_grid_setup
state->area[0] = state->viewsize * state->viewsize
. Now looks much better :) https://hastebin.com/raw/dizagoboxu
I've just tested your changes and i looks like you found it :)
yeah now its pretty close :)
but moments is wrong I have to test it out
I was testing surf_area too, it seems broken too
works when grid spacing is like -g10,10. But on refining like -g100,10 it becomes very wrong.
also the logic only works for boxes
works when grid spacing is like -g10,10. But on refining like -g100,10 it becomes very wrong.
Wouldn't be -g1,1 the refined grid? 100 > 10
i meant like when the grid is refined by half until it reaches the grid_spacing limit
also the logic only works for boxes
You could take the normal vector into account.
can you explain like how normal vector can be used to find surface area?
The surface isn't usual orthogonal to the ray direction. I.e. using ray-thickness * ray*thickness
as the area below the ray would be wrong. Correct?
yep if it doesn't hit the surface at 90 degree then taking the area as ray_thickness * ray_thickness is wrong
Therefore, you need to determine how "skew" the surface below the ray is.
yes
This can be done with the help of the normal vector of the surface at the ray's hit point. The vector product of this vector with the ray's direction vector is equal to the cosine of the angle between these vectors.
And since Pythagoras, ray_thickness * ray_thickness / cos(alpha)
can be used as an approximation for the surface's area around the ray's hit point.
alright sounds good.
Aren't the rays very thin?
if the rays are 1mm thick then we must use -g1,1 to get correct results right?
Aren't the rays very thin?
Indeed :) I mean the grid size.
hmm so -g1,1 is must to get the surface area.
But how to get this normal vector along the surface ?
if the rays are 1mm thick then we must use -g1,1 to get correct results right?
The rays itself have no thickness, I referred to a "ray thickness" value we had at the beginning of your project and which was simply the cell size.
hmm so -g1,1 is must to get the surface area.
No, the "thickness" is simply your actual cell size.
thickness = cell-size is a bit confusing to be honest :D
But how to get this normal vector along the surface ?
Something like m_partition->pt_inhit->hit_normal and m_partition->pt_outhit->hit_normal (for the other side).
thickness = cell-size is a bit confusing to be honest :D
In the terms of api.c it's probable cell_area / cos(alpha)
.
But how to get this normal vector along the surface ?
Something like m_partition->pt_inhit->hit_normal and m_partition->pt_outhit->hit_normal (for the other side).
thanks this helps :)
I don't know where the * 2
comes from there.
I don't know where the
* 2
comes from there.
the other side?
Sorry, no. In prd->optr->o_surf_area[state->curr_view] += (cell_area * 2);
in api.c.
Maybe for the front and back side?
yeah that is what I mean by the other side i.e the exit
But how to get this normal vector along the surface ?
Something like m_partition->pt_inhit->hit_normal and m_partition->pt_outhit->hit_normal (for the other side).
hmm this is deprecated, replaced by RT_HIT_NORMAL
I greped and found this:
RT_HIT_NORMAL( inormal, pp->pt_inhit, pp->pt_inseg->seg_stp, &(ap->a_ray), pp->pt_inflip )
RT_HIT_NORMAL( onormal, pp->pt_outhit, pp->pt_outseg->seg_stp, &(ap->a_ray), pp->pt_outflip )
This looks reasonable.
okay great, I will try it out :)
BTW I added the quiet missed report and required number of hits options.
I had to add a local overlaps list to identify if the hits are zero then it should check if it actually was recorded as overlaps if the hit is zero. else report it ( here is the snippet extracted from gqa - https://hastebin.com/avihedafuc.php)
okay great, I will try it out :)
A surface algorithm may need an additional trick. Test your algorithm with a cube and a sphere. The 3-grid version of the algorithm as it is now shouldn't be able to compute the surface of the cube.
okay great, I will try it out :)
A surface algorithm may need an additional trick. Test your algorithm with a cube and a sphere. The 3-grid version of the algorithm as it is now shouldn't be able to compute the surface of the cube.
ok. I will test it and see how it goes!
This can be done with the help of the normal vector of the surface at the ray's hit point. The vector product of this vector with the ray's direction vector is equal to the cosine of the angle between these vectors.
RT_HIT_NORMAL(inormal, pp->pt_inhit, pp->pt_inseg->seg_stp, &(ap->a_ray), pp->pt_inflip); RT_HIT_NORMAL(onormal, pp->pt_outhit, pp->pt_outseg->seg_stp, &(ap->a_ray), pp->pt_outflip); icos = VDOT(inormal, ap->a_ray.r_dir)/(MAGSQ(inormal)*MAGSQ(ap->a_ray.r_dir)); ocos = VDOT(onormal, ap->a_ray.r_dir)/(MAGSQ(onormal)*MAGSQ(ap->a_ray.r_dir));
here is how I found the cosine angles
hmm in the example you gave you kept the viewsize the same. But its not the case with 1 grid vs 3 grid.
For example a 2M x 2M x 2M cube, mdl_max : (1000,1000,1000) mm and mdl_min: (-1000, -1000, -1000) mm
Span is VSUB2(mdl_max, mdl_min) = (2000, 2000, 2000).
For 3 grid the viewsize is 2000mm and area is 2000x2000mm, so for 50mm gridspacing the grid formed is 40x40.
For 1 grid the viewsize is taken as MAGNITUDE(span) = 3464.101mm and area is 3464.101mm x 3464.101mm, so for 50mm gridspacing the grid formed is 69.28 x 69.28 ~ 69x69.
It doesn't matter if the viewsize is the same or not. With a different viewsize on the 1-view, that just means there is more padding around the outside. (So in my example of 50% of the shots hitting, it would be some smaller percentage on 1-view.) The estimates should still work out approximately within tolerance -- error should be something like the area of half the perimeter cells on average or all the perimeter cells worse case. If they don't, it should be a bug.
Yes Sean, I agree with you. It was a bug -- The area of the grid was kept as the same for both single grid and triple grid.(like this state->area[0] = state->span[1] * state->span[2]) This was fine for 3grid because the are of the grid is same as the state->area.
But in case of single grid the area of the grid is not equal to the state->area. Hence the ratio of missed shots was wrong!
I added the code :
I added this to single_grid_setup
state->area[0] = state->viewsize * state->viewsize
. Now looks much better :) https://hastebin.com/raw/dizagoboxu
to manually set the correct area for single grid and it worked out.
okay great, I will try it out :)
A surface algorithm may need an additional trick. Test your algorithm with a cube and a sphere. The 3-grid version of the algorithm as it is now shouldn't be able to compute the surface of the cube.
you were right! The surf_area of sphere came out thrice the real one for triple grids. But something likecheck surf_area -g10,10 -N1 sphere
gave the surface area correctly.
Also the area always comes out a bit lesser than the correct answer. I found out it was because of the steps in the grid.
When something like -g100mm,100mm
is selected then it shoots 19x19x19 grids. Which gives the surf_area for one view as 19*19*100*100*2 = 7.22m^2
. That gives total area of 3 views as 21.66 m^2. But the correct answer is 24m^2.
Now if the grids were 20x20x20, then the surf_area per view comes out as 20*20*100*100*2 = 8m^2
which is exactly what we needed!
I also investigated the issue with when the grid is refined from something like 100mm to 50mm. The surface area reported was 42m^2 but the correct surf_area is 24m^2.
According to the original code it kept adding the cell_area to the o_surf_area[view] so the when the algorithm is run twice the area is more.
But why is it not twice as the correct surf_area ? It is because on refining we are not shooting all the rays (i.e skipping the already shot rays).
So I did the following changes : https://hastebin.com/iwifusokap.diff
There two things happening : 1) clears the o_area after shooting grids of one gird-size and stores the result to o_surf_area. So it is not double the area.
2) We are also multiplying the o_area with a ratio of total_points/total_shots -- this ratio is 1 for the original gird_spacing but more than 1 for any refined grid. 1.333 in case of when a 100mm grid is refined to 50mm.
The algorithm not only fails for something like a sphere, what about when the object is like a smaller cube is subtracted from a larger cube - the area reported would be the outer-surface area only, the internal area would be ignored.
you were right! The surf_area of sphere came out thrice the real one for triple grids. But something like
check surf_area -g10,10 -N1 sphere
gave the surface area correctly.
The sphere is a more general example for a surface area then the cube, because every part of its surface is visible from any direction, especially from the x, y, and z axis directions. If you think of a small area on the spheres surface, you can hit it with rays from the x, y, and z direction. I.e. if you simply add the areas in you triple grid algorithm you get three times its area.
The cube however is abnormal. Any part of its surface can only be hit by one of the three coordinate axis directions. Therefore, your algorithm only seems to be correct there. Create a turned cube and run your algorithm with this ;)
you were right! The surf_area of sphere came out thrice the real one for triple grids. But something like
check surf_area -g10,10 -N1 sphere
gave the surface area correctly.The sphere is a more general example for a surface area then the cube, because every part of its surface is visible from any direction, especially from the x, y, and z axis directions. If you think of a small area on the spheres surface, you can hit it with rays from the x, y, and z direction. I.e. if you simply add the areas in you triple grid algorithm you get three times its area.
The cube however is abnormal. Any part of its surface can only be hit by one of the three coordinate axis directions. Therefore, your algorithm only seems to be correct there. Create a turned cube and run your algorithm with this ;)
yeah a turned cube will also give incorrect results for sure! :/ well this algorithm is no good then.
I also investigated the issue with when the grid is refined from something like 100mm to 50mm. The surface area reported was 42m^2 but the correct surf_area is 24m^2.
According to the original code it kept adding the cell_area to the o_surf_area[view] so the when the algorithm is run twice the area is more.
But why is it not twice as the correct surf_area ? It is because on refining we are not shooting all the rays (i.e skipping the already shot rays).
So I did the following changes : https://hastebin.com/iwifusokap.diff
There two things happening : 1) clears the o_area after shooting grids of one gird-size and stores the result to o_surf_area. So it is not double the area.
2) We are also multiplying the o_area with a ratio of total_points/total_shots -- this ratio is 1 for the original gird_spacing but more than 1 for any refined grid. 1.333 in case of when a 100mm grid is refined to 50mm.
Hmm, I thought the grid cell size is of relevance for the area, not the number of shots.
And, if you refine the grid by dividing the edges in half, you had to divide the o_surf_area by 4 and shoot at the missing grid points, or?
yeah a turned cube will also give incorrect results for sure! :/ well this algorithm is no good then.
As I said, it needs a trick, like choosing the grid planes randomly in space and looking for the mean of the biggest values.
Hmm, I thought the grid cell size is of relevance for the area, not the number of shots.
And, if you refine the grid by dividing the edges in half, you had to divide the o_surf_area by 4 and shoot at the missing grid points, or?
Hmm now that I think divide by 4 also works! so at the end I can do o_surf_area[view] = o_area[view] then do o_area[view] /= 4. So that the first answer is there in o_surf_area, if we are refining then the new answer can replace old answer.
yeah a turned cube will also give incorrect results for sure! :/ well this algorithm is no good then.
As I said, it needs a trick, like choosing the grid planes randomly in space and looking for the mean of the biggest values.
when you said randomly does it mean not essentially along the 3 axises? and how many grids should we shoot?
when you said randomly does it mean not essentially along the 3 axises? and how many grids should we shoot?
You choose randomly an azimuth and elevation and shoot a grid from this direction. For a first test I would try it with 3 grids. The number of grids could be a parameter of the check function as well.
okay so if I shoot three grids randomly on a sphere, I will get three values for surf_area which could be the same, and I take mean of these three values which gives me the answer but then what did you mean by "the mean of the biggest values" ?
all grids would report me a surf_area value, how to consider which are biggest ?
If a value is much lower than the others then it's likely that this direction wasn't good (missed a part of the surface) and it should be rejected.
However, all values could be good as in case of the sphere.
yep got it :)
now this won't allow us to find internal surf_area right?
You could define a limit, e.g. every value which is smaller than 80% (?) of the maximum will be discarded.
now this won't allow us to find internal surf_area right?
This should work independently of this issue. To get this, you had to follow the ray further on.
even when a smaller cube is subtracted from a larger cube in the centre ?
in that case if a ray goes though the centre would there be two hits?
if that is the case then it should be okay
If a region has a hole, or in case of a torus for example, you get more than one hit from the ray trace. Whenever the ray travels through a solid part of the region you get it as a hit.
alright that sounds good
okk then I will try it out now.
Well that worked :)
https://paste.debian.net/1035316/
I also tried that cubes example that too came to be correct :)
when you said randomly does it mean not essentially along the 3 axises? and how many grids should we shoot?
You choose randomly an azimuth and elevation and shoot a grid from this direction. For a first test I would try it with 3 grids. The number of grids could be a parameter of the check function as well.
I took the number of grids as the number of views so by default it is 3. And can be changed with -N option which is already there to set the number of views
going to commit it then start with glint algorithms
glint seems to shoot the grid differently and has one option to set ray_pt as some random point in the cell
like for particular grid cell, the ray_pt is not necessarily the centre of the grid cell by default
and even if I set the ray_pt as the centre with -c option the outputs of check overlaps and glint does not match for the same grid size and az/el values
and even if I set the ray_pt as the centre with -c option the outputs of check overlaps and glint does not match for the same grid size and az/el values
Hmm, glint -c shoots the rays from the middle of the cells and check from the cells' corners?
(only an idea)
hmm I checked once again - glint -c -g100 -a0 -e0
on cube of sides 8x8x8m centre 0,0,0 the first ray_pt is (4000,-4000,4000) then next is (4000, -3900,4000)
if it were at the centre shouldn't it be (3950, -3950, 3950) and (3950, -3850,3950)
ohh check in single grid shoots from the eye so the ray_pts are different and hence the overlaps list is different
ohh check in single grid shoots from the eye so the ray_pts are different and hence the overlaps list is different
You found it - again :simple_smile:
Just did a detailed analysis now. For grid_spacing of 2000 for a cube of 8x8x8m with centre at (0,0,0)
This is check -a45 -e45 :
Confused on whether to use the exisiting grid setup for single grids in check and add the extra operations provided by glint? But this way if I need to replace the backend of glint with libanalyze API then outputs won't match.
The other way to go is have a totally different grid setup function that would shoot the ray like glint does.
I'm gonna go with different grid setup.
@Saran Narayan it's not as important to match the outputs of glint -- WAY more important that gqa/rtcheck give the same results
it just needs to be able to do all the same checks with similar output reporting as glint
Alright that sounds good :)
@Sean some of the glint options are already in check/gqa like -- vacuum == gqa's gaps, air contiguous == gqa'a adj_air and air first+air last == gqa's exposed air ?
I had added air first and air last's callback functions to api.c but I think exposed air does it already. So I am not adding the first and last air options to check command. I added the unconf_air command to check
@Sean some of the glint options are already in check/gqa like -- vacuum == gqa's gaps, air contiguous == gqa'a adj_air and air first+air last == gqa's exposed air ?
I honestly am not sure -- you'd have to read their respective manual pages and/or read the source code to make sure
I am sure about vacuum and air contiguous by the source code and documentation.
But not so sure about air_first , air_last and exposed_air
according to gqa's documentation for exposed air -- "made to see if the ray encounters air regions before (or after all) solid objects"
but the second line says -- "It also checks to see if the ray moves from a void to an air region."
according to glint's documentation, air_first checks if "The first partition has a nonzero air code" and air_last check if "The last partition has a nonzero air code"
So exposed_air of gqa does air_first + air_last and more.
what surprises me is that I have the code as it is in glint for api.c for last_air and first_air but I am not getting the same results. Totally different results! (different regions)
I was going through rtcheck's code and discovered a bug with grid-setup when -V option was used.
Like normally the defaults are 512x512 width and height and for truck.g g4 the default gird cell width and height comes out be 21.9221mm x 21.9221mm for 512x512 width and height.
But when -V option is used which is used to set the width/height that is the aspect.
like if I do -V 1/2 then the grid cell sizes comes out as 21.9221mm X 43.8422mm but the width and height remains the same as 512x512. Shouldn't it be 256x512 according the cell sizes?
[deleted]
Shouldn't it be 256x512 according the cell sizes?
Sorry, not 256x512 it should be 512x256 to make a square grid
Is this expected behaviour? What exactly is -V option used for ?
I am confused because if I had to replace the back-end of rtcheck with libanalyze/api.c the -V option should work.
I thought if it means width/height then it can related to the cell_width/cell_height ratio hence Iadded an extra variable gridRatio (r71320)
So exposed_air of gqa does air_first + air_last and more.
This sounds about right. I don't think there's a need to track those two separately -- gqa is doing it right combining them.
what surprises me is that I have the code as it is in glint for api.c for last_air and first_air but I am not getting the same results. Totally different results! (different regions)
That said, this is concerning! Can you see if you can figure out why? It casts doubt on the new code doing what we expect.
Is this expected behaviour? What exactly is -V option used for ?
totally expected and intended behavior. -V is making the pixel cells rectangular -- this is important for some output formats.
what surprises me is that I have the code as it is in glint for api.c for last_air and first_air but I am not getting the same results. Totally different results! (different regions)
That said, this is concerning! Can you see if you can figure out why? It casts doubt on the new code doing what we expect.
Ok will try to find it.
I had started to work on rtcheck and using the backend as libanalyze. I had the old file which I worked on analyze_overlaps function. Here is my progress: I tested the basics and it worked. rtcheck.c
All the libanalyze functions happen in rtcheck_do_frame(..)
This way the -M matrix commands should work like cm_ae, cm_viewsize, cm_eyept etc
@Saran Narayan how's your shell scripting? It would be great to validate check against rtcheck and gqa a bit more systematically, like looping over all top-level objects in all our sample .g files, run check vs rtcheck and check volume vs gqa -Av etc, and report any differences found
But rtcheck and gqa are available like executable programs. Can I call MGED commands as executable programs ? Or would it require a wrapper like one for gqa in src/gtools/gqa.c ?
you can run mged commands easily from a script, e.g., mged -c test.g tops
so you could run mged -c test.g check ... args ...
oh cool :slight_smile:
@Sean I just did the comparison of old rtcheck vs new rtcheck (with api.c as base) with multiview matix command on havoc. It came out pretty close :) just two extra overlaps in case -a0 -e30 https://www.diffchecker.com/AkFhnk33
@Sean I just did the comparison of old rtcheck vs new rtcheck (with api.c as base) with multiview matix command on havoc. It came out pretty close :) just two extra overlaps in case -a0 -e30 https://www.diffchecker.com/AkFhnk33
I'd call that a match!
however, false sense of security beware. you have it testing a bunch of azimuths, but the sensitivity is going to be proportional to the sampling density, not necessarily the direction. It'd be good to check a broad range of az/el like you're doing there on havoc, e.g., every 24 or 15 degrees at least through 180 az and el
but then also check default grid, then more refine down to, say, 1mm or less
yep changing the grid density may yield interesting results!
I will get started on the shell script to test these commands.
These are the scripts I have ready :
volume_airs.sh compares the check volume, check gap, check adj_air and check exp_air with gqa's options
rtcheck.sh compares oldrtcheck and new rtcheck for different values of az/el values.
overlaps.sh ompares the ouputs of check overlaps and gqa -Ao triple grid.
Gonna give it a test run. I hope my CPU won't fry with 100% usage
still a lot of work going though the diff files :(
running new rtcheck and old rtcheck twice at 5mm and 50mm spacing for 144 combinations of az/el angles for each object in all the .g files would take very long time..
[duplicated post]
These are the scripts I have ready :
volume_airs.sh compares the check volume, check gap, check adj_air and check exp_air with gqa's options
rtcheck.sh compares oldrtcheck and new rtcheck for different values of az/el values.
overlaps.sh ompares the ouputs of check overlaps and gqa -Ao triple grid.
Gonna give it a test run. I hope my CPU won't fry with 100% usage
@Saran Narayan would you add your scripts to the repo somewhere? just for references/testing -- perhaps in a misc/check subfolder or regress/check subfolder
haven't looked at them yet, but how do you run them typically? we have assets that will churn through all 144 combinations ... really fast.
Umm right now it's just running the commands and redirecting the output to files then I simply use diff command to compare the outputs of say check volume and gqa -Av. And then store the diff to a reports file for later inspection.
The problem with that is the diff file produced will always have some content because of the difference the outputs are printed from these commands (spaces and tabs in the output not the value)
What needs to be done is to use regex to just extract the value and compare them.
I'll try to do that for each command specifically and then compare them right away as they happen.
For something like the overlaps list without the -P 1 option the ordering of the overlaps is not same. So in case of overlaps I need to first extract the data and then sort them. Then compare line by line.
@Sean they are up in the repo now. I used sed to extract the data and compare.
I took some code from the src/tclscripts/checker/check.sh to find the cmd in installed folder.
Running them is simply ./volume.sh for example. The output of script will show which command is running and sed output if any, It will produce some files which will get deleted at the end. A ~_report.txt file is left-off which contains any different results.
thanks I'll take a look!
have you found more differences?
have you found more differences?
Not really, didn't run it extensively. Ran all the tests on g4 and everything looked good except for :
check overlaps and rtcheck because check overlaps reported in gqa -Ao style.
And gqa -Ao reports pairs if seen in reverse as one like for example - /g4/r95 /g4/r93 4 1234mm
rtcheck reported it as /g4/r95 /g4/r93 3 1234mm /g4/r93 /g4/r95 1 12.34mm
So with r71359 check overlaps would report like rtcheck does! but that brings the problem other way around xD check overlaps and gqa -Ao won't match in case of triple grids.
I found rtcheck overlaps reporting better and went with it. Hope that is fine :)
it is fine, that was one of the problems that made us keep rtcheck even after gqa
rtcheck has had better reporting
so when I talk about differences, I should explain that I mean any differences that are not expected or are not directly explained
so if we see A overlaps B and then encounter B overlaps A ... how do the three different tools report it? rtcheck I thought collapsed them into one report (that was related to the bug you found)
so if we see A overlaps B and then encounter B overlaps A ... how do the three different tools report it? rtcheck I thought collapsed them into one report (that was related to the bug you found)
I went through the code for each of the tools:
/g4/r59 and /g4/r63 overlap </g4/r59, /g4/r63>: 13 overlaps detected, maximum depth is 0.000539005mm </g4/r63, /g4/r59>: 3 overlaps detected, maximum depth is 0.000539005mm /g4/r73 and /g4/r69 overlap </g4/r73, /g4/r69>: 5 overlaps detected, maximum depth is 0.000539005mm </g4/r69, /g4/r73>: 9 overlaps detected, maximum depth is 0.000539005mm
rtcheck prints the statistics at the end on how many overlaps it seen, how many are unique etc.
It does not print the ihit or ohit point. Neither does it sort the overlaps in any order.
create_overlap(struct region *r1, struct region *r2) if (r1 < r2) { op->glo_r1 = r1; op->glo_r2 = r2; } else if (r1 > r2) { op->glo_r1 = r2; op->glo_r2 = r1; } else { self overlap }
So there is no case of A overlap B and B overlap A -- it all comes as either A overlap B or B overlap A based on the comparison.
It does not seem to group the overlaps together instead reports them one by one, nor does it report the total count, which rtcheck does.
It does display the ihit and ohit separately, plus there is an option to show the ray_origin (ray->r_pt).
It can also sort the overlaps in the order of volume with -s option.
( sample output with -s and -o option enabled -- https://hastebin.com/voqezehuzu)
for (BU_LIST_FOR (rp, region_pair, &list->l)) { if (bu_strcmp(rp->r.r1->reg_name, r1->reg_name) <= 0) break; } BU_LIST_INSERT(&rp->l, &rpair->l);
I did the documentation work related to check command and is now up on the repo :)
@Sean wiki appears to be gone again :/ getting 404
not really gone, only get 404 after I add something to my log entry.
When I save the edit it opens this link - http://brlcad.org/wiki/User:Sharan.nyn/GSoC18/Log which seems broken
But this works - http://brlcad.org/w/index.php?title=User:Sharan.nyn/GSoC18/Log
@Sean wiki appears to be gone again :/ getting 404
Thanks for reporting it @Saran Narayan .. there's a web service that keeps overwriting the .htaccess which controls the wiki -- trying to find where it's happening, but please just keep reporting when you notice (server is currently taken offline as a result)
I was wondering on what to do with the check_overlaps
command that lives in libged/check_overlaps.c because we have a better version of that command as a sub-command for check
in libged/check/check_overlaps.c.
I could remove it and replace everywhere else as check overlaps
but check_overlaps was my first two weeks worth of work for this project. Though it helped me implement the check command very quickly because of the experience with callback functions :)
but having two commands that essentially does the same is confusing and redundant.
there is one more problem I found -- check
command is blocking the calls to the checker tool that is called with check [-F] [filename]
.
Renaming and updating the documentation related to checker tool should do the trick. I was thinking something like geom_checker or checker_tool.
updating the documentation reminds me, I have to document the newly added overlaps_tool command as well! should I append this to the existing doc/docbook/articles/en/overlap_tool.xml ?
Should I remove all the mentioning of the check.sh script in the overlap_tool.xml file since there is UI replacement for it now :)
(deleted)
sorry for the duplicate messages. Zulip on phone is buggy :sweat_smile:
sorry for the duplicate messages. Zulip on phone is buggy :sweat_smile:
SO buggy!
I could remove it and replace everywhere else as
check overlaps
but check_overlaps was my first two weeks worth of work for this project. Though it helped me implement the check command very quickly because of the experience with callback functions :)
It's a hard thing to learn, but it's good to learn to write code and throw it away. There's actually a rather respected software development method where you write code, delete it, and write it again. You invariably end up with considerably better code the second time around.
but having two commands that essentially does the same is confusing and redundant.
This is the most important consideration. Only one should be kept.
there is one more problem I found --
check
command is blocking the calls to the checker tool that is called withcheck [-F] [filename]
.
Renaming and updating the documentation related to checker tool should do the trick. I was thinking something like geom_checker or checker_tool.
We don't need ... more options. We need consolidation into fewer options.
the script can go away if everything it did is now covered by other commands.
but having two commands that essentially does the same is confusing and redundant.
This is the most important consideration. Only one should be kept.
okay cool. I am on it. Will remove the older check_overlaps now
there is one more problem I found --
check
command is blocking the calls to the checker tool that is called withcheck [-F] [filename]
.
Renaming and updating the documentation related to checker tool should do the trick. I was thinking something like geom_checker or checker_tool.We don't need ... more options. We need consolidation into fewer options.
you misinterpreted me. The checker tool that lives in src/tclscripts/checker/check.tcl has the public proc and entry point named as check. So when I added the libged check command, the checker tool is not working so it needs to be renamed to something else
you misinterpret my response. the checker tool entry point is no longer needed if it's provided for elsewhere ;)
ah that works too :)
umm one quick question.. the check command does not have a -s option to set the size like check_overlaps did.. So the menu options 50x50, 100x100, 256x256 and 512x512 cannot be used with check overlaps.
Any suggestions on the menu option? would something like grid-spacing work? maybe 50mm, 25mm,10mm and 1mm?
I committed like that for now..
er, so you can set the grid cell size but not the grid size?? why not just migrate that code from check_overlaps/rtcheck?
infer the grid spacing
okay will do that :+1:
right now this is how the grid size is set --
width = state->viewsize/cell_width + 0.99; height = state->viewsize/(cell_height * state->aspect) + 0.99;
will add a public function to explicitly mention the grid size and calculate the grid cell size from it like rtcheck does
okay it is done and is up, now making the respective changes to the documentation and archer
humm I am seeing this error whenever I quit from mged -c
with q
-- https://hastebin.com/raw/ikohuvurey
happens only when there is a database loaded
my fault, i’ll fix it soon
i think it should be fixed now
yep that is solved :)
you misinterpret my response. the checker tool entry point is no longer needed if it's provided for elsewhere ;)
@Sean I did it like this : https://hastebin.com/colebuwunu
Were you expecting something like this? or is there any other way?
removing check from check.tcl will break the src/tclscripts/checker/test_checker.tcl.
Hey @Daniel Rossberg I have attempted to make gqa's base as libanalyze/api.c -- gqa.c
I was looking for ways to collect all the work I have done (commits and patches) into one big patch so that I can submit for final evaluation. Any tips on how to do that?
I was looking for ways to collect all the work I have done (commits and patches) into one big patch so that I can submit for final evaluation. Any tips on how to do that?
Hmm, Google recommends a blog post (I would recommend to do this somewhere below https://brlcad.org/wiki/User:Sharan.nyn) with links to the commits you made. I.e. you could link there to your development log page which contains all the links to your commits. You could there include my commits of your patches too.
In addition, you could attach a separate zip or tar file with the most relevant files which you worked on almost exclusively (rtcheck, linged's check command, etc.) to your blog post.
Hmm, Google recommends a blog post (I would recommend to do this somewhere below https://brlcad.org/wiki/User:Sharan.nyn) with links to the commits you made. I.e. you could link there to your development log page which contains all the links to your commits. You could there include my commits of your patches too.
yeah that sounds good. I am already keeping track of my commits on my log page, I will add more details.
In addition, you could attach a separate zip or tar file with the most relevant files which you worked on almost exclusively (rtcheck, linged's check command, etc.) to your blog post.
This also is a good idea.
Thanks for the tips :)
I believe now what remains to do is -- documenting the overlaps_tool.
Hey @Daniel Rossberg I have attempted to make gqa's base as libanalyze/api.c -- gqa.c
:) You file is much smaller then the current one. And, it's desirable to use the libanalyze methods wherever possible.
How does your implementation behave compered to the old one?
works almost fully like old gqa
In the testings I did the order of the outputs does not match like the -- overlap messages
in new implementation it gets printed by bu_log
but old gqa did it with gedp->result str
one more difference is related to overlaps list
Sean had asked me to not keep the stopping of grid refinement like when -g50,2 is mentioned then it should run full till 2mm and then report overlaps
to sum up the order of printing information can be different and some features related to stopping of grid refinement may not work
but the required information like the values are same as the old gqa :)
in new implementation it gets printed by bu_log
??? Shouldn't the result text be written to ged_result_str?
oh not that, the result str i.e the list of overlaps when -Ao is chosen is printed fine
to sum up the order of printing information can be different and some features related to stopping of grid refinement may not work
but the required information like the values are same as the old gqa :)
This sounds good :)
but when other analysis options like -Av is selected and there are overlaps in the geometry they are printed in one line without much info
https://sourceforge.net/p/brlcad/code/HEAD/tree/brlcad/trunk/src/libged/gqa.c#l903
this line of code
since it is done in overlap routine which is handled in api.c I can print it with bu_log
https://sourceforge.net/p/brlcad/code/HEAD/tree/brlcad/trunk/src/libanalyze/api.c#l479 in api.c
here is where I print it in main thread -- https://sourceforge.net/p/brlcad/code/HEAD/tree/brlcad/trunk/src/libanalyze/api.c#l1415
Well, for me it's OK to handle them this way. The overlaps are only of additional information here.
yep! according to man page if such messages are print then the values reported in analysis must be discarded as they can be inaccurate.
I had started to work on rtcheck and using the backend as libanalyze. I had the old file which I worked on analyze_overlaps function. Here is my progress: I tested the basics and it worked. rtcheck.c
All the libanalyze functions happen in rtcheck_do_frame(..)
here is rtcheck I messaged this on aug 2 -- this works pretty good for havoc.g
I had done some more changes to the file locally, but there is a power cut here so I cannot access my PC to get that file :/
I had done some more changes to the file locally, but there is a power cut here so I cannot access my PC to get that file :/
Maybe, you should call it a day?
to keep the functionality of the -M commands had to keep the rtip. Plus there are commands that set the viewsize, eye model etc. Hence instead of passing of the AE angle and using the setup ae on libanalyze side I kept the setup AE on rt side and passed the information with a function that sets the viewsize, eye model and orientation matrix
This sounds reasonable.
I had done some more changes to the file locally, but there is a power cut here so I cannot access my PC to get that file :/
Maybe, you should call it a day?
yep :D alright then will get back to you
you misinterpret my response. the checker tool entry point is no longer needed if it's provided for elsewhere ;)
@Sean I did it like this : https://hastebin.com/colebuwunu
Were you expecting something like this? or is there any other way?
something like that, yes, maybe even a way to kick off the gui still from the command-line but from the new tool and only when tk is available?
Sean had asked me to not keep the stopping of grid refinement like when -g50,2 is mentioned then it should run full till 2mm and then report overlaps
yes, gqa is stupid in this regard -- it will halt at 50 if it finds overlaps. it only refines if it finds none which means if you want to guarantee overlaps down to Xmm (which is the common case), you need to run it over and over or specify differently -g2,2; ideally, it would be better if it displayed overlaps incrementally and interruptibly for a specification like 50,2
@Sean I did it like this : https://hastebin.com/colebuwunu
Were you expecting something like this? or is there any other way?something like that, yes, maybe even a way to kick off the gui still from the command-line but from the new tool and only when tk is available?
okay I committed it, To run the gui directly I had the usage like overlaps_tool [-F] file.overlaps
.
I tried to find out way to execute it only tk is available. I ran into the winset command which returns a pathname of the mged window. If I keep $parent as the output from winset command, then it would throw an error when tk is not available (ran mged -c in nu mode).
humm also the global variable mged_players only exists when tk is available (?)
I had done some more changes to the file locally, but there is a power cut here so I cannot access my PC to get that file :/
@Daniel Rossberg the only change I did was add the analyze_set_ncpu(state, npsw)
because -P option was not working without
I don’t recall what the best mechanism is for determining whether there’s a GUI, but I do know there are a few places in the code that do that ( maybe search on ‘catch’ ).
hmm could not find anything with catch
but this seems to be what we are looking for ? https://sourceforge.net/p/brlcad/code/HEAD/tree/brlcad/trunk/src/tclscripts/mged/overlap.tcl#l492
I added the report for the project here - https://brlcad.org/wiki/User:Sharan.nyn/GSoC18/Report
@Daniel Rossberg I still have to add the files which I worked on excursively, where should I upload them ?
@Daniel Rossberg I still have to add the files which I worked on excursively, where should I upload them ?
Can you attach the zip or tar file to your report at https://brlcad.org/wiki/User:Sharan.nyn/GSoC18/Report similar to the Overlaps_tool_final.png image?
okay will do
should I submit the rtcheck and gqa as patches ?
should I submit the rtcheck and gqa as patches ?
In my opinion you could submit them but @Sean knows more about the people who are actually using these tools and how they are influenced by the changes.
hmm yeah, that is why I did not commit them. :)
Or maybe I will just include them as tar in my report?
hmm ".gz" is not a permitted file type. Permitted file types are png, jpg, jpeg, svn, gif, svg, pdf.
on wiki
You can always include them in the tarball. It's a result of you GSoC work. There is only a little question if it can be used in the official distribution.
hmm
".gz" is not a permitted file type. Permitted file types are png, jpg, jpeg, svn, gif, svg, pdf.
:thinking: Do you have access to another server where you can upload the tarball and include a link to it in your report?
no, I don't have access to any other servers :/. I can upload it google drive.
Yes, creating a public folder in Google Drive is among the Good Examples in Google's guidelines.
okay :) that would be perfect.
done!
just completed the documentation work related to overlaps tool
added some screenshots
Nice Report :)
thanks
That's strange: Calling check overlaps
makes my laptop sound like an old floppy disk. It has an SSD.
I didn't get you
My laptop starts making noise which reminds me on a floppy disk drive. I've no explanation for it. Beside the fan the computer shouldn't have moving parts.
Maybe a side effect on the audio subsystem caused by an unusual CPU load? (no idea)
That's strange indeed :D
Nothing what should bother you :wink:
added some screenshots
awesome! i wonder if there's a way we could take advantage of your animated gifs ... thoughts? maybe a gif that works as a screenshot but then is animated when viewed / embedded in html browser form.
@Saran Narayan I think check_overlaps is still registered somewhere? there was a "make test" failure on the 'regress-mged' test that mentions it.
@Saran Narayan I think check_overlaps is still registered somewhere? there was a "make test" failure on the 'regress-mged' test that mentions it.
hmm it seems to be working for me.. Output of make test
: https://hastebin.com/fiwemofawe
added some screenshots
awesome! i wonder if there's a way we could take advantage of your animated gifs ... thoughts? maybe a gif that works as a screenshot but then is animated when viewed / embedded in html browser form.
That sounds like a good idea ;)
I replaced the second image with the GIF file and it animates when I open the HTML file in chrome.
But I didn't get the idea of it working as a screenshot :thinking:, Are there any other ways these documentation files are viewed ?
@Saran Narayan I think check_overlaps is still registered somewhere? there was a "make test" failure on the 'regress-mged' test that mentions it.
hmm it seems to be working for me.. Output of
make test
: https://hastebin.com/fiwemofawe
hum, maybe an out-of-date tclIndex or something on my end. I'll retry with a fresh build. thanks for checking @Saran Narayan
I replaced the second image with the GIF file and it animates when I open the HTML file in chrome.
Cool.
But I didn't get the idea of it working as a screenshot :thinking:, Are there any other ways these documentation files are viewed ?
Yes, some of the docs are viewable in the documentation browser (which I think you have to run from archer to get the full interface). it has an embedded html browser, but it's very simple
I added the entry in the toc.html file and viewed it from archer's doc browser. It worked as an image in the documentation browser but works as a GIF in chrome when the html file is opened directly.
I committed the changes
having difficulties with internet connectivity and power due to floods in my state, I will be back after everything is normal here :)
PNG didn't work in chrome? That doesn't sound right...
The other way round: They say GIF isn't working in BRL-CAD's documentation browsers.
GIF works but it does not animate, looks still like a image
@Saran Narayan GIF won't work in the native Tcl/Tk help browser (or at least, I wouldn't expect it to).
Last updated: Nov 15 2024 at 00:49 UTC