So, here is the generic volume implementation. Right now, I'm shooting only in one direction (axis aligned) and there is no tolerance specified or no grid refinement done.
I will add the grid refinement and (if required) the multiple views. I just wanted to know if the additions in "include/analyze.h" are up to the mark. :)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hmm,
Why have you added a new function pointer entry to rt_functab? And why does this new function need a name?
The analyze_volume() function doesn't look so bad. It gets a pointer to the database and an object name and should return the volume, but why doesn't it return the volume? I know, ft_volume() returns the volume by value. This is probable because the return values of rt_functab functions are reserved for error codes. But does this hold for your libanalyse too? (OK, yes if you use it directly in rt_functab.)
Next you need a connection to the rt_functab. You don't need it for primitives which already have a better (faster and more pricise) volume function. For every other element a rt_generic_volume((fastf_t * , const struct rt_db_internal *) adapter function could be the solution.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hi Daniel,
I tried to have a look at the places you mentioned and well, here are some comments :
The new entry to rt_functab is there because none of the existing signatures in rt_functab allow me to use the function analyze_volume(). I need to have a database instance and the object name in my function. They are used for getting raytracing instance and for doing rt_gettree() repsectively. Now, if I use the present ft_volume entry, I will have a rt_db_internal only, which is not what I require for analyze_volume().
Yes, I can return the value to an adapter function calling analyze_volume(). That can be changed. I didn't bother to write an adapter function because I was adding an entry for signature of analyze_volume() in rt_functab and hence returning the volume by value. My bad here -- I can return the volume and write an adapter function for calling it and this adapter function will return volume by value.
For the connection to rt_functab -- yes, you're right -- I will use the generic function only for primitives which don't have a better / precise method implemented already. But as I said before, the signature of rt_generic_volume(fastf_t *, const struct rt_db_internal *) won't work unless there's a way to get the database instance from the rt_db_internal and somehow get the object name as well.
So, there are two ways that I can see here :
I change the signature of ft_volume so that it takes a database instance, a fastf_t pointer and an object name. I would convert the database instance to rt_db_internal and everything would work just as before.
I add an entry in rt_functab so that it entails the signature of my analyze_volume() function and we will be able to achieve the aims : that analyze_volume() returns the volume and connection with rt_functab will be done by a rt_generic_volume, just that it will have a different signature now.
The first one includes a lot of intricate work, including changing the functions for volume in all the primitives, their table, the functab and the analyze.c.
The second one, on the other hand is simpler to work with I guess. This is what I think.
I'd like to hear your views on it.
Regards,
Kalpit
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Sorry for my late answer but I had to think about it and made some tests.
You may be right about that rt_db_internal isn't sufficient to do a ray-trace. However, starting from the ray-trace related functions in rt_functab you could still be able to shoot rays. I.e. you have a BRL-CAD database element, so you have its rt_functab functions and with this its ray-trace routines. Based on which parameters are really necessary to shoot a ray you could revise the ft_volume() parameters. This is only an idea, which can be pointless besides.
That's why it could be reasonable to remember your original proposal where you wanted to write a volume function for the C++ interface. I.e. get Object.h/Object.cpp and implement a Volume() function there which uses ft_volume() from the functab, and if it's NULL it will use libanalyze.
Daniel
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
It's certainly possible to create a raytrace context (rt_i) given a database context (db_i) and vice versa. It's also possible to create a db_i given an rt_db_internal. The only problem that comes to mind is trying to shoot the rt_db_internal for a comb, which is really just a graph of names (strings). You need a database context that resolves those names in order to shoot the shape it symbolically represents. In that sense, just having a single rt_db_internal is insufficient to do a ray trace, unless you're only dealing with primitives (solid leaf nodes).
Given there will always be the possibility for there to be primitives that won't have an ft_volume() callback defined, I think the original proposal was on track. You could add a struct db_i parameter to ft_volume() so that implementing a callback for combs is possible, but there will still be some entities that will not have it (e.g., brep, dsp).
Side-comment: Note that the functab entry for comb does not (yet) have prep()/shot() callbacks defined since that's notionally what rt_shootray() does. It'd be pretty simple to implement, though.
Side-comment #2: rt_generic_volume() should match the signature of ft_volume. Adding a struct db_i is okay, adding a string object name would not be okay...
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I have kept 3 views now (like gqa) and added a volume tolerance that is checked for the criteria to terminate the shooting of rays. The grid refinement factor is kept at 2. You can test the patch with the "analyze" command.
I have changed analyze.c just for testing purposes. If this implementation seems reasonable enough, I'll submit just the changes related to volume implementation.
Should I now start implementing the Volume() function in C++ interface? I will keep improving this volume implementation when needed.
Some questions:
- Why did you put the GET_NEXT_ROW macro (and others) in the public libanalyze header?
- SETUP_RAYTRACING_CONTEXT is a very complex, stacked macro which is hard to debug. Why have you chosen this way, and not to do it without a macro or with a function?
- Why is struct current_state in include/analyze.h?
Daniel
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hello!
So, I have made the required changes and you can find the updated patch attached. Please review it ASAP so that I can get working on the changes without any further delay.
Oh. I had added it as my function setup_raytracing_context() needed a memset() call. I had implemented it initially in analyze.h itself. So, it's still sitting there.
Right, I got it. I can include it in api.c right away.
And I'll review my patches better before submitting them here.
Thanks.
Kalpit
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hello!
So, I have finally implemented the new API with volume and centroid working. Surface Area is still left. I have attached the new patch. The patch contains 3 files -- api.c, volume.c and centroid.c -- all in src/libanalyze. There are also some small changes in density.c as I had used it to print the imported density table and found some errors.
Note #1 : centroid works for now as I have hard-coded the GIFTmater index to 2 -- this is because the sample table GQA_SAMPLE_DENSITIES doesn't have an entry for indices 0 and 1.
Note #2 : For the centroid to work, you will have to copy GQA_SAMPLE_DENSITIES to your $HOME directory with the name ".density".
Please review this patch ASAP. It's testing requires building MGED only -- this will work for any revision you're presently on.
Pipeline in BRIEF : ged_analyze() calls analyze_check() to check if the object to be analyzed has a primitive-specific method implemented. Objects not having such specific methods -- their names are stored and sent to setup_raytracing_context(). It does the gqa-style shooting rays on grid and fills the object table and returns a raytracing context. This raytracing context is used by analyze_volume() and analyze_centroid() for providing the required analysis results. Those objects that have specific methods -- analyze_do() is called for them. That's it.
How to use in C++ interface : setup_raytracing_context() is called if the object's ft_volume/centroid/surf_area entry is NULL -- analyze_volume/centroid/surf_area follows as per requirement. Else the funtab entry is used for the analysis.
Daniel,
Sorry for not being available for almost one week. My Fall semester just started so I got a bit busy with the course / project registrations and stuff.
Anyways, I started working on the libanalyze functionality again and implemented the surface area part as I have understood it.
I have attached the updated patch and I will upload the Caller functions patch including Surface Area as well, soon. Please review it ASAP.
Side note : My patch regarding the arb8 surface area is still pending. I had updated it and performed the tests that you told me to. Please do check it whenever possible.
You have 4 identical definitions of the struct per_region_data. However, the only one needed is in src/libanalyze/api.c. Therefore you should remove any other definition/declaration.
Is there a rule when you use extern in a function declararion?
There is a huge bunch of static variables. Are they really necessary?
Regards,
Daniel
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Daniel,
Yes, the definition of struct per_region_data is required only in api.c right now. I have removed the multiple definitions. Thank you.
No, there's no rule as such. Yes, functions are extern by default, but I like to add the keyword anyways. So, that's why it's there.
The huge bunch of static variables is needed as I have to cover all analysis performed by gqa eventually. So, I haven't removed all the variables which are not used. I have commented some of them.
The updated patch is attached.
NOTE : Right now, the libanalyze code calculates the tolerance itself and it doesn't take in a user-defined tolerance. But adding it won't be a big deal.
The static variables could be an issue. In case of a well defined executable as gqa it's no problem, but in a library. They aren't save for multithread programs. Isn't it possible to add them to the current_state (c_state) struct?
Daniel
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Daniel,
I can add the UNUSED variables to the current_state struct. If it's going to create problems for multithreaded programs, then this could be done.
If there is anything else that needs to be changed, I'd like your comments on it ASAP. Tomorrow is the hard deadline and I need to start doing the documentation properly. If this is the final thing, I'll update the patch and upload.
Regards,
Kalpit
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Kalpit,
What do you mean with "add the UNUSED variables to the current_state struct". Shouldn't the unused variables be removed and the used ones moved to current_state?
And yes, I've another wish: Centroid() should work without a densioty file. In this case everything has the same density.
Regards,
Daniel
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Daniel,
I meant that the variables that are not used right now, I would comment them out / add them to the c_state. Okay, but that wouldn't be a good idea yes. I have removed the variables that are not used and cleaned up the code. Only those which are used are now there and some of them which could be moved to c_state have been moved there.
I hope this won't create problems for multithreading now.
And about the Centroid() : So, I have considered the default material as Aluminium 7xxx series, as I read that aluminium alloys are used a lot in military equipment and 7xxx is one of the two entries in the file GQA_SAMPLE_DENSITIES. Hence, Centroid() now works without a density file as well.
I will add the grid refinement and (if required) the multiple views. I just wanted to know if the additions in "include/analyze.h" are up to the mark. :)
Hmm,
Why have you added a new function pointer entry to rt_functab? And why does this new function need a name?
The analyze_volume() function doesn't look so bad. It gets a pointer to the database and an object name and should return the volume, but why doesn't it return the volume? I know, ft_volume() returns the volume by value. This is probable because the return values of rt_functab functions are reserved for error codes. But does this hold for your libanalyse too? (OK, yes if you use it directly in rt_functab.)
Next you need a connection to the rt_functab. You don't need it for primitives which already have a better (faster and more pricise) volume function. For every other element a rt_generic_volume((fastf_t * , const struct rt_db_internal *) adapter function could be the solution.
Hi Daniel,
I tried to have a look at the places you mentioned and well, here are some comments :
*, const struct rt_db_internal*) won't work unless there's a way to get the database instance from the rt_db_internal and somehow get the object name as well.So, there are two ways that I can see here :
The first one includes a lot of intricate work, including changing the functions for volume in all the primitives, their table, the functab and the analyze.c.
The second one, on the other hand is simpler to work with I guess. This is what I think.
I'd like to hear your views on it.
Regards,
Kalpit
Sorry for my late answer but I had to think about it and made some tests.
You may be right about that rt_db_internal isn't sufficient to do a ray-trace. However, starting from the ray-trace related functions in rt_functab you could still be able to shoot rays. I.e. you have a BRL-CAD database element, so you have its rt_functab functions and with this its ray-trace routines. Based on which parameters are really necessary to shoot a ray you could revise the ft_volume() parameters. This is only an idea, which can be pointless besides.
That's why it could be reasonable to remember your original proposal where you wanted to write a volume function for the C++ interface. I.e. get Object.h/Object.cpp and implement a Volume() function there which uses ft_volume() from the functab, and if it's NULL it will use libanalyze.
Daniel
It's certainly possible to create a raytrace context (rt_i) given a database context (db_i) and vice versa. It's also possible to create a db_i given an rt_db_internal. The only problem that comes to mind is trying to shoot the rt_db_internal for a comb, which is really just a graph of names (strings). You need a database context that resolves those names in order to shoot the shape it symbolically represents. In that sense, just having a single rt_db_internal is insufficient to do a ray trace, unless you're only dealing with primitives (solid leaf nodes).
Given there will always be the possibility for there to be primitives that won't have an ft_volume() callback defined, I think the original proposal was on track. You could add a struct db_i parameter to ft_volume() so that implementing a callback for combs is possible, but there will still be some entities that will not have it (e.g., brep, dsp).
Side-comment: Note that the functab entry for comb does not (yet) have prep()/shot() callbacks defined since that's notionally what rt_shootray() does. It'd be pretty simple to implement, though.
Side-comment #2: rt_generic_volume() should match the signature of ft_volume. Adding a struct db_i is okay, adding a string object name would not be okay...
Hello!
I have kept 3 views now (like gqa) and added a volume tolerance that is checked for the criteria to terminate the shooting of rays. The grid refinement factor is kept at 2. You can test the patch with the "analyze" command.
I have changed analyze.c just for testing purposes. If this implementation seems reasonable enough, I'll submit just the changes related to volume implementation.
Should I now start implementing the Volume() function in C++ interface? I will keep improving this volume implementation when needed.
Regards,
Kalpit
Some questions:
- Why did you put the GET_NEXT_ROW macro (and others) in the public libanalyze header?
- SETUP_RAYTRACING_CONTEXT is a very complex, stacked macro which is hard to debug. Why have you chosen this way, and not to do it without a macro or with a function?
- Why is struct current_state in include/analyze.h?
Daniel
Hello!
So, I have made the required changes and you can find the updated patch attached. Please review it ASAP so that I can get working on the changes without any further delay.
Regards,
Kalpit
A short question for a better understanding: Why does analyze.h need to include string.h?
Daniel
Oh. I had added it as my function setup_raytracing_context() needed a memset() call. I had implemented it initially in analyze.h itself. So, it's still sitting there.
Right, I got it. I can include it in api.c right away.
And I'll review my patches better before submitting them here.
Thanks.
Kalpit
Hello!
So, I have finally implemented the new API with volume and centroid working. Surface Area is still left. I have attached the new patch. The patch contains 3 files -- api.c, volume.c and centroid.c -- all in src/libanalyze. There are also some small changes in density.c as I had used it to print the imported density table and found some errors.
Note #1 : centroid works for now as I have hard-coded the GIFTmater index to 2 -- this is because the sample table GQA_SAMPLE_DENSITIES doesn't have an entry for indices 0 and 1.
Note #2 : For the centroid to work, you will have to copy GQA_SAMPLE_DENSITIES to your $HOME directory with the name ".density".
Please review this patch ASAP. It's testing requires building MGED only -- this will work for any revision you're presently on.
Pipeline in BRIEF : ged_analyze() calls analyze_check() to check if the object to be analyzed has a primitive-specific method implemented. Objects not having such specific methods -- their names are stored and sent to setup_raytracing_context(). It does the gqa-style shooting rays on grid and fills the object table and returns a raytracing context. This raytracing context is used by analyze_volume() and analyze_centroid() for providing the required analysis results. Those objects that have specific methods -- analyze_do() is called for them. That's it.
How to use in C++ interface : setup_raytracing_context() is called if the object's ft_volume/centroid/surf_area entry is NULL -- analyze_volume/centroid/surf_area follows as per requirement. Else the funtab entry is used for the analysis.
Regards,
Kalpit
Daniel,
Here is the updated patch. :)
Kalpit
Daniel,
Sorry for not being available for almost one week. My Fall semester just started so I got a bit busy with the course / project registrations and stuff.
Anyways, I started working on the libanalyze functionality again and implemented the surface area part as I have understood it.
I have attached the updated patch and I will upload the Caller functions patch including Surface Area as well, soon. Please review it ASAP.
Side note : My patch regarding the arb8 surface area is still pending. I had updated it and performed the tests that you told me to. Please do check it whenever possible.
Regards,
Kalpit
Kalpit,
You have 4 identical definitions of the struct per_region_data. However, the only one needed is in src/libanalyze/api.c. Therefore you should remove any other definition/declaration.
Is there a rule when you use extern in a function declararion?
There is a huge bunch of static variables. Are they really necessary?
Regards,
Daniel
Daniel,
Yes, the definition of struct per_region_data is required only in api.c right now. I have removed the multiple definitions. Thank you.
No, there's no rule as such. Yes, functions are extern by default, but I like to add the keyword anyways. So, that's why it's there.
The huge bunch of static variables is needed as I have to cover all analysis performed by gqa eventually. So, I haven't removed all the variables which are not used. I have commented some of them.
The updated patch is attached.
NOTE : Right now, the libanalyze code calculates the tolerance itself and it doesn't take in a user-defined tolerance. But adding it won't be a big deal.
Regards,
Kalpit
The static variables could be an issue. In case of a well defined executable as gqa it's no problem, but in a library. They aren't save for multithread programs. Isn't it possible to add them to the current_state (c_state) struct?
Daniel
Daniel,
I can add the UNUSED variables to the current_state struct. If it's going to create problems for multithreaded programs, then this could be done.
If there is anything else that needs to be changed, I'd like your comments on it ASAP. Tomorrow is the hard deadline and I need to start doing the documentation properly. If this is the final thing, I'll update the patch and upload.
Regards,
Kalpit
Kalpit,
What do you mean with "add the UNUSED variables to the current_state struct". Shouldn't the unused variables be removed and the used ones moved to current_state?
And yes, I've another wish: Centroid() should work without a densioty file. In this case everything has the same density.
Regards,
Daniel
Daniel,
I meant that the variables that are not used right now, I would comment them out / add them to the c_state. Okay, but that wouldn't be a good idea yes. I have removed the variables that are not used and cleaned up the code. Only those which are used are now there and some of them which could be moved to c_state have been moved there.
I hope this won't create problems for multithreading now.
And about the Centroid() : So, I have considered the default material as Aluminium 7xxx series, as I read that aluminium alloys are used a lot in military equipment and 7xxx is one of the two entries in the file GQA_SAMPLE_DENSITIES. Hence, Centroid() now works without a density file as well.
Regards,
Kalpit