| 00:40.00 | Notify | 03BRL-CAD:starseeker * 57409 NIL: Branch for experimenting with the popt option handling library. |
| 01:03.46 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 01:09.48 | Notify | 03BRL-CAD:starseeker * 57410 (brlcad/branches/popt/INSTALL brlcad/branches/popt/configure and 2 others): Integrate popt option handling library version 1.16 from http://rpm5.org/files/popt/ |
| 02:10.26 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 03:20.21 | brlcad | starseeker: depends on the walker, but I highly doubt it |
| 03:20.39 | brlcad | should be able to get a dp on the fly, lots of code does exactly that |
| 03:21.56 | brlcad | dp = DB_FULL_PATH_CUR_DIR(pathp); |
| 04:09.55 | Notify | 03BRL-CAD:phoenixyjll * 57411 brlcad/trunk/src/librt/comb/comb_brep.cpp: Check the db_i pointer. |
| 04:46.07 | Notify | 03BRL-CAD:phoenixyjll * 57412 (brlcad/trunk/src/librt/comb/comb_brep.cpp brlcad/trunk/src/librt/primitives/brep/brep_debug.cpp): Some renaming - ip => dbip; brep_conversion => single_conversion |
| 05:02.26 | Notify | 03BRL-CAD:phoenixyjll * 57413 (brlcad/trunk/src/libged/brep.c brlcad/trunk/src/librt/comb/comb_brep.cpp brlcad/trunk/src/librt/primitives/brep/brep_debug.cpp): db => dbip, and mark const for some variables. |
| 05:54.03 | Notify | 03BRL-CAD:phoenixyjll * 57414 brlcad/trunk/src/libbrep/boolean.cpp: Check 3D distance (not included in ON_Brep::IsValid()) when looking for seam trims. |
| 06:26.33 | *** join/#brlcad d_rossberg (~rossberg@66-118-151-70.static.sagonet.net) |
| 06:32.07 | *** join/#brlcad vladbogo (~vlad@188.25.239.225) |
| 06:33.09 | Notify | 03BRL-CAD:phoenixyjll * 57415 brlcad/trunk/src/librt/comb/comb_brep.cpp: Perform Xform for the leaf. |
| 06:38.21 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 06:46.56 | *** join/#brlcad PrezKennedy (~DarkCalf@173.231.40.99) |
| 07:54.12 | *** join/#brlcad Ch3ck_ (~Ch3ck@195.24.220.16) |
| 08:12.59 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 08:13.22 | Ch3ck_ | brlcad: corrected the bn_poly_sub() uploaded patch. I've also finished with the interface. Submitted patch already awaiting review ;) |
| 08:59.20 | *** join/#brlcad caen23 (~caen23@92.83.184.3) |
| 09:08.16 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 09:45.59 | Notify | 03BRL-CAD:phoenixyjll * 57416 brlcad/trunk/src/libbrep/boolean.cpp: Generate the connectivity graph for the new solid (after evaluation) |
| 09:48.35 | Notify | 03BRL-CAD Wiki:Phoenix * 6088 /wiki/User:Phoenix/GSoc2013/Reports: /* Week 12 */ |
| 10:02.50 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 10:40.22 | *** join/#brlcad Ch3ck_ (~Ch3ck@195.24.220.16) |
| 10:40.54 | *** join/#brlcad witness__ (uid10044@gateway/web/irccloud.com/x-otrloazsutjbgdui) |
| 11:27.26 | Notify | 03BRL-CAD:tbrowder2 * 57417 brlcad/trunk/doc/docbook/system/man3/en/CMakeLists.txt: remove 3rd party doc from our doc tree |
| 11:40.58 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 13:26.01 | *** join/#brlcad witness__ (uid10044@gateway/web/irccloud.com/x-aospgmtgctafjpdc) |
| 14:15.12 | Notify | 03BRL-CAD:starseeker * 57418 NIL: Looks like we won't need this branch after all. |
| 14:29.29 | Notify | 03BRL-CAD:starseeker * 57419 brlcad/trunk/src/other/CMakeLists.txt: Add minimal file set from tclap 1.2.1 to support experiments in improvements to option parsing. There is a config.h file, so we may need to add CMake logic for a few system tests if long long and/or strstream detection prove necessary. |
| 15:21.56 | Notify | 03BRL-CAD:tbrowder2 * 57420 brlcad/trunk/src/util/dsp_add.c: add info; remove unneeded semicolon |
| 15:41.53 | Notify | 03BRL-CAD:tbrowder2 * 57421 brlcad/trunk/src/util/CMakeLists.txt: add copy of dsp_add.c for conversion to C++ and using tclap arg processing |
| 15:49.52 | Notify | 03BRL-CAD:tbrowder2 * 57422 brlcad/trunk/src/util/dsp_add_t.cpp: add casts to avoid invalid conversion errors |
| 15:54.35 | Notify | 03BRL-CAD:tbrowder2 * 57423 brlcad/trunk/src/util/dsp_add_t.cpp: add casts to avoid errors: deprecated conversion from string constant to 'char*'; last of C++ fixes for good build |
| 16:11.07 | Notify | 03BRL-CAD:tbrowder2 * 57424 brlcad/trunk/src/util/dsp_add_t.cpp: comment out tclap header due to problems with tclap config--needs work |
| 16:32.38 | brlcad | awesome: http://www.youtube.com/watch?v=CUx2ypHQJO0 |
| 16:35.46 | Ch3ck_ | brlcad: i've corrected the bn_poly_sub() patch |
| 16:39.03 | Notify | 03BRL-CAD:n_reed * 57425 brlcad/trunk/src/util/dsp_add_t.cpp: const param type should remove need to cast string literal arguments. Can skip printing both NULL and empty string args. Make exit behavior more consistent, using bu_exit where convienent. |
| 16:39.04 | brlcad | Ch3ck_: I know |
| 16:39.11 | brlcad | Ch3ck_: will be checking it later today |
| 16:39.24 | brlcad | what was the problem? |
| 16:44.59 | Ch3ck_ | well i gave wrong args |
| 16:45.03 | Ch3ck_ | like test values |
| 16:45.14 | Ch3ck_ | brlcad: i've also finished with the pull interface |
| 16:45.29 | brlcad | wrong args to what? |
| 16:45.38 | Ch3ck_ | i've created the documentation integrated the pull correctly; but pulls only combinations for now |
| 16:45.56 | Ch3ck_ | well I the values from octave were wrongly input |
| 16:46.19 | Ch3ck_ | so thats why the test kept coming up wrong |
| 16:46.24 | Ch3ck_ | it was my mistake there |
| 16:46.28 | brlcad | you mean the reference values? |
| 16:46.31 | Ch3ck_ | yes |
| 16:46.38 | Ch3ck_ | I mixed them up |
| 16:46.55 | brlcad | ok |
| 16:47.20 | Ch3ck_ | well finished with the documentation and apropos, synopsis of the pulll |
| 16:47.25 | Ch3ck_ | integrated patch already |
| 16:47.34 | Ch3ck_ | the patch is on sf |
| 16:47.59 | Ch3ck_ | Well concerning the my implementation of the pull_leaf routine since i'll have to support all primitives |
| 16:48.11 | Ch3ck_ | what approach will i need to implement all of them |
| 16:48.29 | Ch3ck_ | well posted a question on mailing list for the primitives i'll need some assistance with ;) |
| 16:49.05 | brlcad | okay sounds good, one step at a time |
| 16:49.31 | brlcad | the approach will probably be nearly identical for all of the primitives that have a V parameter |
| 16:49.49 | Ch3ck_ | yeah but there are some which do not have that i think |
| 16:49.58 | brlcad | it'll obviously be more tricky for the ones that don't |
| 16:50.06 | Ch3ck_ | yeah |
| 16:50.08 | brlcad | about a 1/3rd iirc |
| 16:50.26 | Ch3ck_ | well concerning my use of the switch is a bad design choice |
| 16:50.46 | brlcad | one possibility |
| 16:50.52 | Ch3ck_ | what's the best approach for implementing the pull leaf for each primitive |
| 16:51.08 | Ch3ck_ | should i implement functions corresponding to each routine |
| 16:51.09 | Ch3ck_ | ? |
| 16:51.14 | brlcad | the approach is going to involve defining a new callback interface |
| 16:51.20 | Ch3ck_ | ok |
| 16:51.24 | brlcad | a function that will apply to all primitives |
| 16:51.38 | Ch3ck_ | so how will it look like? |
| 16:51.52 | brlcad | it'll look however you make it look :) |
| 16:52.00 | brlcad | but it should look like the existing functions |
| 16:52.09 | brlcad | they all have a similar patter |
| 16:52.11 | brlcad | pattern |
| 16:52.25 | brlcad | stepping back though, there may be another way at least for traslation |
| 16:52.43 | Ch3ck_ | yeah |
| 16:52.58 | Ch3ck_ | thats mostly V param in almost all primitives |
| 16:53.19 | brlcad | no, you misunderstand |
| 16:53.24 | Ch3ck_ | what about building the 4x4 matrix transform like scale and rotation from the other aspects of the primitive |
| 16:54.08 | brlcad | every primitive already defines a bounding box function |
| 16:54.13 | Ch3ck_ | ok |
| 16:54.27 | brlcad | you could use that information to extract a translation, putting the center of the bounding box at the origin |
| 16:55.23 | Ch3ck_ | so how'll that look like? |
| 16:55.41 | brlcad | if starseeker got OOBB's working, you could even use that for rotation and scaling |
| 16:55.59 | Ch3ck_ | OOBB's? |
| 16:56.12 | Ch3ck_ | what's that? |
| 16:58.08 | brlcad | http://en.wikipedia.org/wiki/Bounding_volume |
| 16:58.16 | brlcad | object bounding boxes |
| 16:58.22 | brlcad | object-oriented |
| 16:58.25 | zero_level | waves to brlcad. |
| 16:58.39 | brlcad | hi zero_level, avail to resume |
| 16:59.35 | brlcad | Ch3ck_: the question is whether we want to unroll a translation based on a primitives "data origin" or it's "natural origin" |
| 16:59.41 | brlcad | they are two distinctly separate concepts |
| 16:59.52 | brlcad | take a simple box, for example, an RPP |
| 17:00.08 | brlcad | it's data origin is the first corner. that's "V". |
| 17:00.19 | zero_level | brlcad : do you have the link ? |
| 17:00.21 | brlcad | it's natural origin, however, is the boxes centerpoint |
| 17:00.54 | brlcad | that centerpoint is (length/2,width/2,height/2) |
| 17:00.59 | zero_level | http://paste.kde.org/p60667585/http://paste.kde.org/p60667585/ |
| 17:01.05 | zero_level | nevermind |
| 17:01.06 | zero_level | http://paste.kde.org/p60667585/ |
| 17:01.30 | brlcad | the bounding box method will find the natural centerpoint, what you were working on using V will find the data origin |
| 17:01.41 | brlcad | both are valid, but it's an interesting question |
| 17:01.53 | brlcad | zero_level: yep, I've had that window open for days |
| 17:02.15 | Ch3ck_ | is listening |
| 17:02.34 | brlcad | Ch3ck_: i'm done :) |
| 17:02.47 | Ch3ck_ | ok |
| 17:02.48 | brlcad | Ch3ck_: that's the point, that there are two possible "origins" |
| 17:03.02 | brlcad | implementing either will be a set of challenges |
| 17:03.10 | Ch3ck_ | ok |
| 17:03.42 | Ch3ck_ | so based on the OOBB I could get all the information about the primitive right? |
| 17:03.44 | brlcad | you can implement the "natural origin" using the existing bbox callback function now, and only need to do so once for all primitives (i.e., no need for per-primitive functions) |
| 17:04.04 | brlcad | our current bbox() function gives you an AABB, which is only useful for translation |
| 17:04.31 | brlcad | at this point, I think translation is probably all we need to try and encompass |
| 17:04.40 | Ch3ck_ | ok |
| 17:04.41 | brlcad | so you can just use the existing bbox() function |
| 17:04.43 | Ch3ck_ | will look into it |
| 17:04.48 | Ch3ck_ | ok |
| 17:04.51 | zero_level | alright. So the first point is simple discussion about log messages. |
| 17:05.10 | brlcad | Ch3ck_: I suggest writing a little test.c program that opens a .g, gets some geometry and calculates the bbox center |
| 17:05.36 | brlcad | zero_level: yep, do you have an example you can point me towards? |
| 17:05.57 | Ch3ck_ | will do |
| 17:05.58 | brlcad | that question as stated is unanswerable without more context |
| 17:06.25 | zero_level | brlcad : I revisited this issue and there are two types of log messages(large charecterization) |
| 17:06.34 | brlcad | what should be returned depends entirely on where you are in the code |
| 17:07.18 | zero_level | the examples are lying all in the code. |
| 17:07.25 | zero_level | but lets see specific |
| 17:07.31 | zero_level | libicv/crop.c |
| 17:07.32 | brlcad | ~dict lying |
| 17:08.12 | brlcad | okay, good example |
| 17:08.34 | brlcad | so our context is that we're in libicv |
| 17:08.37 | zero_level | l:52 |
| 17:08.38 | brlcad | which is a LIBRARY |
| 17:08.43 | zero_level | yes |
| 17:08.51 | brlcad | a library should never shut down an application |
| 17:09.20 | zero_level | do you recommend, it even when wrong arguments are passed. |
| 17:09.45 | brlcad | uhm, YES! |
| 17:09.48 | brlcad | think about it |
| 17:09.54 | zero_level | ok, |
| 17:09.55 | brlcad | you're using this library |
| 17:10.04 | brlcad | somehow a 0 is specified for a size |
| 17:10.11 | brlcad | and the APP terminates! |
| 17:10.21 | brlcad | i mean for our src/util programs, who cares |
| 17:10.49 | brlcad | but imagine if libicv were integrated into a different program, say Firefox or X11 |
| 17:11.04 | brlcad | would terminating the application make any sense?? |
| 17:11.09 | zero_level | ok. |
| 17:11.13 | zero_level | Got your point |
| 17:11.14 | brlcad | have to think bigger picture |
| 17:11.46 | zero_level | for eg. in libicv/filter.c |
| 17:11.53 | brlcad | now the only exception is when you have a check that is almost certainly an indication of corruption |
| 17:12.28 | brlcad | if you detect that something is horribly horribly wrong, something that should NEVER happen, then terminating can be the safest course of action |
| 17:12.40 | brlcad | but even for that, you wouldn't call bu_exit(), you'd call bu_bomb() |
| 17:13.06 | zero_level | ok. filter.c, l:250 I have returned NULL in case of error after bu_log. |
| 17:13.08 | zero_level | ok. |
| 17:13.15 | brlcad | also for logging, lets leave the function names out of the logging messages, nobody except the devs care ;) |
| 17:13.31 | zero_level | everywhere ? |
| 17:13.35 | zero_level | ok. |
| 17:13.42 | brlcad | I know you're probably just following the pattern elsewhere in our code, but it's something we're trying to get away from |
| 17:13.55 | brlcad | make the message something that makes sense to a user |
| 17:14.11 | brlcad | if it's a critical failure, start the message with "ERROR: " |
| 17:14.41 | zero_level | ok. With my experience I will classify the logging and errors as following. |
| 17:14.54 | zero_level | critical error : bu_bomb. |
| 17:15.34 | brlcad | there's a difference though |
| 17:15.35 | zero_level | marginal error return (-1) forint based and (NULL) for icv_image_t* based. |
| 17:15.39 | zero_level | and |
| 17:15.43 | brlcad | again bu_bomb() only if you detect CORRUPTION |
| 17:15.54 | brlcad | I don't think you have any corruption detection measures in place |
| 17:16.11 | brlcad | getting passed a NULL pointer or negative size is not a corruption |
| 17:16.12 | zero_level | for library based : with function name and error message. |
| 17:16.14 | brlcad | it's a bad application |
| 17:16.55 | zero_level | ok. |
| 17:17.41 | zero_level | so what do we conclude in issue 1 ? |
| 17:18.10 | brlcad | I don't think "marginal" is well defined |
| 17:18.22 | brlcad | it's a matter of inputs and outputs |
| 17:18.39 | brlcad | you need to make sure inputs are always what you expect them to be |
| 17:19.06 | brlcad | for example, icv_get_kernel() in filter.c doesn't check if kern or offset are NULL |
| 17:19.10 | zero_level | and if they are not? Do i bomb ? or pass a log message ? |
| 17:19.13 | brlcad | so it can (and eventually will) crash |
| 17:19.46 | brlcad | so what to do depends on expectation |
| 17:20.00 | brlcad | you marked it HIDDEN, so you control all the callers |
| 17:20.44 | brlcad | for that, any NULL parameter is a library bug because only the library can access that function |
| 17:21.14 | brlcad | if it were NOT hidden, you'd just print an ERROR and return a failure/null/empty/nothing |
| 17:21.18 | zero_level | yes because I call it only twice in the same file. and also the code structure maintains that the parameters are intact ? |
| 17:21.32 | zero_level | sure. |
| 17:21.34 | brlcad | since it is HIDDEN, you can decide but it should still check |
| 17:21.58 | zero_level | Then Let me write the conclusion of this discussion. |
| 17:22.47 | brlcad | long term, HIDDEN functions sometimes become non-HIDDEN, which is why *every* function should still always check the inputs are what are expected |
| 17:23.09 | brlcad | if you detect a NULL pointer and can make the callers behave reasonably, you should |
| 17:23.14 | brlcad | if you cannot, you can bomb |
| 17:23.19 | zero_level | 1) the parameters check should be done correctly. If this is a bad parameter (unexpected) use some default with error or pass error with bomb or error with null (-1) |
| 17:23.45 | zero_level | your point taken as 2) |
| 17:24.37 | zero_level | 3) Dont use function name for error messages. If it is strong error just write ERROR |
| 17:24.42 | zero_level | is anything left ? |
| 17:25.47 | zero_level | Moving to issue#2 |
| 17:25.51 | brlcad | check all parameters |
| 17:26.08 | brlcad | zero_level: hold on |
| 17:26.12 | zero_level | ok. |
| 17:26.41 | brlcad | so you also need some way to *detect* a bad parameter (beyond a NULL pointer) |
| 17:26.58 | brlcad | your icv_get_kernel() function is a prime example |
| 17:27.16 | zero_level | do you mean based on heights and sizes (as in icv_filter3(..) ) |
| 17:27.28 | zero_level | or based on size as in icv_crop in crop.c |
| 17:29.01 | brlcad | I don't understand, arent' those the same? |
| 17:29.35 | brlcad | icv_crop() has ton's of validation issues |
| 17:29.42 | brlcad | (because it has no validation) |
| 17:30.43 | brlcad | I can crash the program just by passing a NULL pointer or a negative value |
| 17:31.14 | zero_level | ok. So adding validation is the key here. |
| 17:31.27 | zero_level | because I understand it will make the library stable. |
| 17:31.45 | brlcad | all PUBLIC should validate ALL parameters before doing any work |
| 17:32.26 | zero_level | I think icv_crop is a best exammple to understand. |
| 17:32.27 | brlcad | you should also validate any combinations that are used that might result in a bad result, such as dividing by zero |
| 17:32.32 | zero_level | a) where to bomb. |
| 17:32.42 | zero_level | b) where to use default |
| 17:32.44 | brlcad | it's public API so it should not bomb |
| 17:32.55 | zero_level | c) where to use default value. |
| 17:33.02 | zero_level | ok. |
| 17:33.09 | zero_level | so a us out of question. |
| 17:33.16 | zero_level | c/us/is |
| 17:33.34 | brlcad | it should certainly report an error |
| 17:33.48 | brlcad | just not .. terminate the application |
| 17:33.49 | Notify | 03BRL-CAD:starseeker * 57426 NIL: Will probably be better to structure this with a subdirectory... |
| 17:33.55 | brlcad | let the app decide that |
| 17:34.31 | brlcad | the question is more interesting/harder for HIDDEN functions |
| 17:34.39 | brlcad | since you have more expections |
| 17:34.48 | brlcad | my point earlier is that you need to be able to detect when something is wrong |
| 17:35.02 | brlcad | so look at icv_get_kernel() |
| 17:35.20 | brlcad | just looking at that now, there's almost certainly a memory corruption (a segfault) |
| 17:35.40 | brlcad | and just checking if kern or offset are NULL would not detect it |
| 17:35.43 | Notify | 03BRL-CAD:starseeker * 57427 (brlcad/trunk/src/other/tclap/Arg.h =================================================================== and 695 others): Move the headers into the subdirectory |
| 17:36.12 | Notify | 03BRL-CAD:mohitdaga * 57428 brlcad/trunk/src/libicv/TODO: Add a TODO item for validation of input arguments. |
| 17:37.36 | brlcad | zero_level: so best starting point for now is probably baby steps .. go over EVERY function and make sure you test whether all your pointers are non-NULL and sizes are >0 where it makes sense, along with any other argument checks that make sense |
| 17:38.12 | brlcad | still before we move on, since I think I see a segfault ... you should see it too |
| 17:42.20 | zero_level | in ? |
| 17:56.05 | brlcad | icv_get_kernel |
| 17:56.49 | brlcad | this is exactly why it's good to always do data validation, even on hidden functions |
| 17:57.05 | brlcad | because bugs can be anywhere, will be everywhere, given enough time |
| 17:58.11 | brlcad | you have three arguments to icv_get_kernel(), you should be able to narrow it down to just one that has a bug (or prove me wrong) |
| 18:05.08 | zero_level | brlcad : I dont see any bug. |
| 18:05.16 | zero_level | Also bwfilter works fine. |
| 18:05.20 | zero_level | which uses icv_filter. |
| 18:05.50 | zero_level | the get_kernel is primarly made to service both icv_filter and icv_filter3 |
| 18:06.17 | zero_level | and kern has variable size. |
| 18:06.35 | zero_level | in both these functions. |
| 18:06.39 | brlcad | zero_level: you effectively already check filter_type with the switch statement |
| 18:06.44 | brlcad | and you have a default handler |
| 18:07.15 | brlcad | a program working is NEVER an indication of whether there is a bug or not, especially for segment violations |
| 18:07.23 | zero_level | ok. |
| 18:07.43 | zero_level | alright i see your point. |
| 18:07.45 | brlcad | rather a WORKING program is never an indication :) |
| 18:08.13 | brlcad | plenty of bugs can go masked indefinitely (for decades), but they're still real bugs |
| 18:08.21 | brlcad | so not filter_type |
| 18:08.59 | brlcad | offset is a pointer, you obviously don't check the value, but it's clearly only used as a pointer to a double (a single double value) |
| 18:09.21 | brlcad | looking at the two callers, you pass the address of a local variable |
| 18:09.37 | zero_level | yes. |
| 18:09.43 | zero_level | Is that a bad practice ? |
| 18:09.47 | brlcad | so if that's ever NULL, it would be an indication of stack corruption (something worthy of a bu_bomb) |
| 18:10.08 | brlcad | no, it's fine |
| 18:10.49 | brlcad | that leave us with good old 'kern' |
| 18:11.16 | brlcad | looking at icv_get_kernel(), it's being indexed as an array |
| 18:11.32 | brlcad | how big is it? |
| 18:15.22 | Notify | 03BRL-CAD:mohitdaga * 57429 brlcad/trunk/src/libicv/filter.c: If the filter specified is not present in the library, we should not consider filtering the image. Instead return with an error. |
| 18:17.31 | zero_level | it can be of two sizes for different function |
| 18:17.40 | zero_level | icv_filter (9) |
| 18:17.52 | zero_level | icv_filter3 (27) |
| 18:18.27 | zero_level | but although get kernel is designed as a temporary function which holds the library of filters. |
| 18:19.06 | Notify | 03BRL-CAD:starseeker * 57430 (brlcad/trunk/src/other/CMakeLists.txt brlcad/trunk/src/other/tclap/config.h.in and 5 others): Get dsp_add_t working for testing purposes - the util inclusions of tclap header directories will go away once libbu wraps it. |
| 18:19.11 | zero_level | In the later stages I plan to make a advanced get_kernel function which gives filters of a specified size. |
| 18:19.20 | zero_level | and type. |
| 18:19.35 | zero_level | will have to review his signal processing concepts here. |
| 18:20.03 | zero_level | and the fact we are dealing with double data. we could probably have some nice filtering reasults. |
| 18:20.35 | brlcad | slow down horsie |
| 18:20.46 | brlcad | it can be two sizes |
| 18:20.57 | brlcad | but those sizes are triggered by a type, not the function |
| 18:21.03 | zero_level | Indeed the icv_filter and icv_filter3 are designed such that they accept variable length filters. |
| 18:21.14 | zero_level | at present the size is fixed to default size. |
| 18:21.15 | brlcad | rather, the indexinging is triggered by a type, not the function |
| 18:21.36 | zero_level | brlcad : yes. |
| 18:21.58 | brlcad | and that type is passed through from public API |
| 18:22.10 | zero_level | right. |
| 18:22.19 | zero_level | is there an issue ? |
| 18:22.33 | brlcad | icv_filter(7, kern, &offset); |
| 18:23.07 | zero_level | ~dict horsie |
| 18:23.19 | brlcad | slow your horse down |
| 18:23.36 | zero_level | ;) |
| 18:23.44 | brlcad | means you're not paying attention closely enough, going to fast to see the problem |
| 18:24.25 | brlcad | if a user is specifying the type, and that type is unchecked, you have a potential segfault |
| 18:24.40 | brlcad | they might even be misusing the API |
| 18:24.49 | brlcad | icv_filter(ICV_FILTER_3_NULL, kern, &offset); |
| 18:25.25 | zero_level | brlcad : I see the point. |
| 18:25.40 | zero_level | But icv_filter just has two arguments. |
| 18:25.58 | brlcad | you obviously can add some stuff in icv_filter() and/or icv_filter3(), but ideally you ALSO add some sort of check/protection inside icv_get_kernel() |
| 18:26.15 | brlcad | and to do that, you need to know the size of kern since it's an array |
| 18:26.23 | zero_level | You are suggesting that some naughty guy can mix the two types of filters. |
| 18:26.32 | zero_level | we need to check that. |
| 18:26.40 | zero_level | you have a point here. |
| 18:26.44 | brlcad | not even necessarily naughty |
| 18:26.47 | brlcad | code changes over time |
| 18:27.03 | brlcad | but this is exactly where bugs evolve, how they usually begin |
| 18:27.04 | zero_level | brlcad++ |
| 18:27.43 | brlcad | another item ... a function should not start with icv_ unless it's public API documented in include/icv.h |
| 18:27.57 | brlcad | and public api should always have the prefix |
| 18:28.06 | brlcad | that way, it's very clear what it public API and what is not |
| 18:28.20 | Notify | 03BRL-CAD:mohitdaga * 57431 brlcad/trunk/src/libicv/filter.c: Remove debug flag. |
| 18:29.03 | zero_level | ok. |
| 18:30.00 | Notify | 03BRL-CAD:mohitdaga * 57432 brlcad/trunk/src/libicv/TODO: Add a TODO item to check for the mixup of two types of filters in filter.c |
| 18:30.40 | zero_level | brlcad : do we move to issue#2 ? |
| 18:30.49 | brlcad | bullet #2: you have have non-public, private API, that is not HIDDEN |
| 18:31.20 | brlcad | create a private header in src/libicv, #include "./yourheader.h", and use the functions |
| 18:32.42 | zero_level | I think i couldnt convey what i wanted to. |
| 18:32.55 | zero_level | did you see encoding.c ? |
| 18:33.07 | brlcad | yes? |
| 18:33.17 | zero_level | also bw.c ? |
| 18:33.38 | zero_level | ok, I see your point. |
| 18:33.56 | brlcad | yeah, technically what you did in bw.c is wrong |
| 18:34.09 | brlcad | invalid ... a little surprised the compiler didn't complain |
| 18:34.21 | zero_level | no! it works. |
| 18:34.30 | zero_level | but that is not the proof. |
| 18:34.33 | brlcad | it shouldn't .. what is an "extern static" ? |
| 18:34.43 | brlcad | the compiler is just defaulting to extern |
| 18:35.03 | brlcad | more than likely, you're getting lucky because gcc doesn't care |
| 18:36.11 | brlcad | the extern declaration says there will be a uchar2double() symbol later ... and the function is compiled static but it's symbol is getting included because nothing in encoding.c uses it (i.e., got lucky) |
| 18:36.30 | brlcad | the compiler has the option to not include a symbol marked as static |
| 18:36.56 | zero_level | ok. So other way as suggested by you is to keep them in encoding.h |
| 18:37.02 | brlcad | no |
| 18:37.04 | zero_level | and include encoding.h . |
| 18:37.20 | zero_level | I mean declare them in encoding.h . |
| 18:37.32 | brlcad | you DECLARE them in encoding.h, you'd remove HIDDEN from their definition in encoding.c |
| 18:37.46 | ``Erik | zero_level: filter.c:259 "return -1;" in a function returning a pointer? |
| 18:37.59 | Notify | 03BRL-CAD:mohitdaga * 57433 brlcad/trunk/src/libicv/filter.c: Change the name of icv_get_kernel -> get_kernel (private function) |
| 18:38.08 | brlcad | zero_level: BUT ... more to say on that |
| 18:38.14 | brlcad | these conversion functions seem wonky to me |
| 18:38.40 | brlcad | if you think of every function as being a plugin type, these are currently outliers |
| 18:38.44 | brlcad | ~dict outlier |
| 18:38.53 | Notify | 03BRL-CAD:mohitdaga * 57434 brlcad/trunk/src/libicv/filter.c: icv_filter3 returns a NULL. |
| 18:39.16 | zero_level | ``Erik r57434 solves that. |
| 18:39.24 | zero_level | thanks. |
| 18:40.21 | zero_level | brlcad : please suggest imporvements. |
| 18:41.08 | zero_level | I must confess, I am novice. |
| 18:42.12 | brlcad | well if we ignore the names for a second and look at what they're doing, the first is basically taking char data and scaling it into the "native library representation" |
| 18:42.48 | brlcad | the second is basically taking the "native library representation" and scaling it into char data |
| 18:43.00 | zero_level | ok. |
| 18:43.23 | brlcad | that gives me two thoughts |
| 18:43.54 | brlcad | 1) this either should be public API so format writers can use it, OR ... |
| 18:44.37 | brlcad | 2) it should live with (one or more) formats specific to their needs (e.g., bw_to_double) |
| 18:45.24 | brlcad | I have a new image file format, .awesome, and I'm writing a libicv plug-in extension |
| 18:46.12 | brlcad | I need to convert my image.awesome into libicv representation, so I will need to define a function that does exactly that |
| 18:47.51 | zero_level | Alright : Will make them public. |
| 18:48.24 | brlcad | are you understanding or just taking the easy route? |
| 18:48.46 | zero_level | In case you have such awesome thing in place. Consider telling to zero_level. He will include in icv |
| 18:48.50 | zero_level | ;) |
| 18:49.04 | zero_level | yes. got your point. |
| 18:49.23 | Notify | 03BRL-CAD:starseeker * 57435 brlcad/trunk/src/conv/step/g-step/ON_Brep.cpp: Testing with simple planar breps seems to suggest that this flag needs to be reversed... |
| 18:49.51 | zero_level | But my thought was because we plan to put the formats in icv. in GSOC and after that. So I thought we might not need them. |
| 18:50.08 | brlcad | it's not as simple as putting icv_ on them though |
| 18:50.15 | brlcad | they are out of place as they are currently defined |
| 18:50.35 | zero_level | Never thought that some other might create "xyz" format and want to use our lib. |
| 18:50.46 | brlcad | e.g., why uchar2double and DATA2uchar? shouldn't that be either uchar2data or double2uchar? |
| 18:51.07 | zero_level | but now want to consider that point. |
| 18:51.32 | brlcad | and if you're going to have uchar, what about char, short, ushort, long, unlong, long long, ulong long, ... |
| 18:51.43 | brlcad | raises questions as API |
| 18:52.53 | zero_level | actually never thought of making them public.And wanted to make as the need arises in png. |
| 18:52.59 | zero_level | or any other formats. |
| 18:53.11 | brlcad | but my point is are they really limited to just one type (e.g., bw) |
| 18:53.44 | zero_level | Which indeed brings us to the other point. Do we need to make them public ? |
| 18:54.15 | brlcad | exactly, it depends |
| 18:54.45 | brlcad | datawise pix is bw*3, but just because it happens to work with the same function doesn't mean that it needs to be public API |
| 18:54.54 | brlcad | it's still an encoding/decoding operation for bw and pix respectively |
| 18:55.06 | brlcad | zero_level: so here's my biggest concern |
| 18:55.12 | brlcad | modularity |
| 18:55.47 | brlcad | I think whatever we end up with, a file format should be defined by one or more files in a directory that are independent |
| 18:56.15 | brlcad | and as an author, it becomes *very* easy to see how the library is extended for a different format |
| 18:56.29 | brlcad | whether that author is you or me or someone external |
| 18:56.48 | brlcad | given enough time, all code becomes code someone else wrote :) |
| 18:59.27 | zero_level | I think i agree. |
| 18:59.54 | zero_level | lets make those notations in doxygen commants in encoding.c |
| 19:00.09 | zero_level | about which functions will work in which fileformat. |
| 19:00.35 | brlcad | zero_level: grep -E '(if|while|for|switch|return)\(' * |
| 19:00.52 | brlcad | style errors |
| 19:01.42 | zero_level | thanks for pointing that out. |
| 19:02.12 | zero_level | adding that to TODO. |
| 19:02.22 | zero_level | I will finish all the work in TODO. |
| 19:02.24 | zero_level | :) |
| 19:03.58 | brlcad | it might turn out that encoding.c can go away |
| 19:04.15 | brlcad | that it'd become just an encoding function in bw |
| 19:04.26 | brlcad | or part of bw_read()/bu_write() |
| 19:04.42 | brlcad | OR |
| 19:05.00 | brlcad | that it becomes part of ICV private API |
| 19:05.28 | brlcad | where functions are defined for converting from each of the intrinsic C types to libicv's native format |
| 19:05.50 | zero_level | alright. |
| 19:05.55 | brlcad | zero_level: another way to think of this ... lets say we want to change LIBICV to not use 0.0-1.0 doubles as the internal format |
| 19:06.10 | brlcad | consider what all would need to change |
| 19:06.26 | brlcad | we want to get that list of things that would need to change down to a bare minimum |
| 19:06.58 | brlcad | if you put bw into a subdir, I wouldn't want anything to change in that subdir if the internal format changed |
| 19:07.01 | Notify | 03BRL-CAD:mohitdaga * 57436 brlcad/trunk/src/libicv/TODO: Add TODO item for stylistic measures |
| 19:07.09 | brlcad | i.e., each type is not aware of the internal format |
| 19:07.19 | brlcad | should not be |
| 19:08.42 | zero_level | brlcad : I get your point. |
| 19:08.46 | brlcad | zero_level: while you're working on TODO items, icv.h could use some attention.. |
| 19:09.08 | brlcad | unclear why icv_rot() is the first thing we're introduced to :) |
| 19:09.53 | brlcad | towards modularity, need to think about how ICV_IMAGE_FORMAT can go away |
| 19:10.44 | brlcad | (if I need to add a new format, I should be able to define everything in one directory like src/libicv/ppm ... without needing to edit a header (i.e., could even be a runtime extension) |
| 19:12.05 | brlcad | zero_level: shall we move on to bullet #3? |
| 19:12.15 | zero_level | Support dynamically loading formats (e.g., define a plug-in API). ? |
| 19:12.24 | brlcad | absolutely |
| 19:12.26 | zero_level | brlcad : just a moment. |
| 19:12.33 | zero_level | ok . |
| 19:12.44 | brlcad | if that's what is required to think about modularity correctly |
| 19:12.54 | brlcad | whether it's actually dynamically loaded is irrelevant |
| 19:13.02 | brlcad | if it cannot be, it's not modular enough |
| 19:13.14 | zero_level | bullet#2 is easy now. |
| 19:13.34 | zero_level | c/2/3 |
| 19:13.48 | zero_level | Just a commen and y/n |
| 19:14.00 | zero_level | s/commen/comment |
| 19:15.25 | brlcad | well I question that function's existence as public API :) |
| 19:15.43 | brlcad | the naming convention is separate from the ICV_IMAGE_FORMAT names |
| 19:16.01 | brlcad | conceptually, it sounds like an icv_pix2bw() function |
| 19:16.16 | brlcad | with a scaling factor |
| 19:16.24 | zero_level | brlcad : do you mean icv_rgb2gray ? |
| 19:16.30 | brlcad | yes |
| 19:16.36 | zero_level | hahah :D |
| 19:16.57 | zero_level | your question is harsh. ;) |
| 19:17.06 | brlcad | I didn't ask a question yet ;) |
| 19:17.24 | brlcad | rather, I guess I did there at the beginning |
| 19:17.30 | zero_level | brlcad : This is needed because it can convert any rgb image into grayscale image. |
| 19:18.31 | brlcad | I think this is part of the disconnect, though |
| 19:18.33 | zero_level | Later color_space might contain icv_rgb2hsv or icv_rgb2ycbcr and.. |
| 19:18.54 | zero_level | any implied image loaded to icv container. |
| 19:19.07 | zero_level | s/question/questioning |
| 19:19.26 | brlcad | except that gray is not really a color space, is it? |
| 19:19.44 | zero_level | well it can be considered as pne. |
| 19:19.44 | brlcad | at least, how's it different from bw_write() |
| 19:19.55 | zero_level | it is. |
| 19:20.21 | zero_level | bw_write takes an icv_container and writes a file in bw format. |
| 19:21.02 | zero_level | icv_rgb2gray takes an icv container with color_channels 3 and converts that to gray scale image. |
| 19:21.27 | zero_level | that is gray scale icv cotnainer with color channel 1 |
| 19:21.51 | zero_level | so if you have an image with 3 container and you want to write bw format output. |
| 19:21.52 | brlcad | so they both take an icv_container |
| 19:21.58 | brlcad | and both write out 1 color channel |
| 19:22.00 | brlcad | what am I missing? |
| 19:22.10 | zero_level | icv_rgb2gray doesnt write. |
| 19:22.18 | zero_level | it converts and returns the container. |
| 19:22.45 | brlcad | besides the file operation is there a difference? |
| 19:22.56 | zero_level | yes. |
| 19:24.23 | zero_level | also, if you take a icv_container with rgb data and want to write a bw image. It will call icv_rgb2gray_ntsc(...) |
| 19:25.22 | zero_level | brlcad : I am able to explain ? |
| 19:30.18 | *** join/#brlcad caen23 (~caen23@92.81.191.54) |
| 19:30.59 | brlcad | yes, but then does that need to be public API or is it private? |
| 19:31.20 | zero_level | PUBLIC. |
| 19:31.24 | brlcad | sounds like it might be private to me, just like the encoding |
| 19:31.56 | brlcad | something I'd use to implement a new format (like .jpeg) but not necessarily a new app (jpeg2pix) |
| 19:32.41 | zero_level | let me give you an example. |
| 19:33.00 | zero_level | Let suppose I built a tank in our cad-tool. |
| 19:33.20 | zero_level | But then my boss wanted me to get the resul in gray scale image. |
| 19:34.17 | zero_level | I have an option in future to make a specific flag which will convert the resultant image to gray scale. |
| 19:34.40 | zero_level | which can be written to any format. jpeg, bw,bmp, png. |
| 19:39.47 | brlcad | okay, that's convincing |
| 19:40.07 | brlcad | but then rgb2gray sounds like it may be too specific |
| 19:40.18 | brlcad | there are NxM possible conversions |
| 19:40.34 | brlcad | that specific one is more of a channel scaling function |
| 19:40.43 | brlcad | so perhaps it could be generalized as such |
| 19:40.45 | zero_level | but the point is what all we need. |
| 19:41.11 | zero_level | Implementation wise is not an issue. |
| 19:41.12 | brlcad | icv_channels(image, num_channels) |
| 19:41.50 | zero_level | No. |
| 19:42.11 | zero_level | because we are looking at color space changes. |
| 19:42.20 | brlcad | that sounds like a completely separate issue |
| 19:42.40 | zero_level | suppose tommorow I have something for an alpha channel . |
| 19:42.47 | brlcad | which is I think why rgb2gray seems overly confusing to me right now |
| 19:43.15 | zero_level | I cannot use icv_channels to magically change to 4 channel image. |
| 19:43.23 | brlcad | why not? |
| 19:43.32 | zero_level | brlcad : I can help you remove confusions. |
| 19:43.33 | brlcad | obviously not with those two simple arguments |
| 19:43.50 | zero_level | ok. |
| 19:44.03 | zero_level | where are u stuck ? |
| 19:44.12 | brlcad | I'm not stuck |
| 19:44.17 | zero_level | :D |
| 19:44.21 | brlcad | i'm saying that the name and scope is misleading |
| 19:45.08 | zero_level | scope I gave an example which you agreed to. |
| 19:45.16 | zero_level | name i am open to change. |
| 19:45.30 | brlcad | the name with that *scope* |
| 19:45.32 | zero_level | Because I am not good at naming things. :) |
| 19:45.34 | brlcad | mixing the notion of channels and color space information just sounds wrong |
| 19:45.42 | zero_level | ok. |
| 19:45.56 | brlcad | I agree that having a user tool want to output, say, a b&w png file would be useful |
| 19:46.06 | zero_level | are they mixed ? |
| 19:46.13 | brlcad | that in code would be a n-channel to 1-channel reduction |
| 19:46.21 | brlcad | well you're saying they are |
| 19:46.26 | brlcad | rgb2hsv |
| 19:46.34 | zero_level | ok. |
| 19:46.40 | zero_level | So that bothers you. |
| 19:46.57 | zero_level | Do you take gray scale as a color space ? |
| 19:47.07 | brlcad | from an API design, yeah it seems mixing two concepts in an inconsistent way |
| 19:47.30 | zero_level | If yes than forget channels. and lets use color spaces. |
| 19:47.40 | brlcad | I could have gray scale as one channel or three or N |
| 19:47.57 | brlcad | that's more a question of data replication |
| 19:48.00 | zero_level | ok. |
| 19:48.19 | zero_level | I see your point. |
| 19:48.35 | zero_level | I think I communicate very poorly. |
| 19:48.45 | zero_level | sorry for that. :) |
| 19:48.53 | brlcad | no, it's fine |
| 19:48.58 | brlcad | we're making progress |
| 19:49.09 | brlcad | only three of eight bullets, but progress ;) |
| 19:49.22 | brlcad | if you need to go, we can pick this up later |
| 19:50.29 | zero_level | brlcad : lets finish this point. |
| 19:50.38 | zero_level | I need to sleep. But can wait. |
| 19:50.51 | brlcad | there's plenty of time to sleep when you're dead ;) |
| 19:50.52 | zero_level | just ensuring that we dont need to start over. |
| 19:50.56 | brlcad | sure |
| 19:51.15 | zero_level | so yes. |
| 19:51.25 | brlcad | so datawise, there are two things happening as I understand it |
| 19:51.36 | zero_level | Let me explain regarding the two public funtions in color_space.c |
| 19:51.38 | brlcad | the icv representation has N channels representing an image, yes? |
| 19:51.43 | zero_level | yes. |
| 19:52.59 | zero_level | icv_gray2rgb : just copies the pixel 3 times and outputs a rgb image (supposedly we cannot convert gray image to rgb) |
| 19:53.49 | zero_level | icv_rgb2gray : combines the three channel image as per the weights specified and produces a gray space (1 channel image) |
| 19:53.55 | zero_level | is this fine ? |
| 19:53.59 | brlcad | sure |
| 19:54.08 | zero_level | now as per the question goes. |
| 19:54.45 | brlcad | data-wise, I think this gets at low-level ICV operations |
| 19:55.14 | zero_level | earlier icv_rgb2gray used another field which marked the presence of method. |
| 19:55.17 | brlcad | there are certain fundamental operations going on that I think can be fully generalized |
| 19:55.23 | zero_level | but later I made that a macro. |
| 19:56.22 | brlcad | sure |
| 19:56.44 | zero_level | generalized ? |
| 19:56.49 | brlcad | right |
| 19:56.57 | zero_level | where ? |
| 19:56.58 | brlcad | so it gets at the heart of what icv_rgb2gray() is doing |
| 19:57.02 | zero_level | can you point an example |
| 19:57.04 | brlcad | to the ICV data |
| 19:57.18 | brlcad | say I have an ICV image |
| 19:57.29 | zero_level | is listening |
| 19:57.33 | brlcad | lets fully ignore alpha channels for the sake of discussion |
| 19:58.15 | brlcad | that ICV data has some number of channels, say N |
| 19:58.29 | brlcad | data-wise and generalized, several operations come to mind |
| 19:58.40 | brlcad | want to delete a channel, for example, easy |
| 19:59.24 | brlcad | want to add a channel .. and when you add, a decision will need to follow -- is the added zero, 1's, a copy of an existing channel (perhaps scaled), an average (perhaps scaled) of multiple channels |
| 19:59.34 | brlcad | want to replicate data across channels |
| 20:00.00 | brlcad | icv_gray2rgb() is easily described in that context |
| 20:00.45 | zero_level | brlcad : you have a point about that. |
| 20:00.49 | brlcad | I think as an API, it would make perfect sense for ICV to define low-level operations like that |
| 20:01.06 | brlcad | upon which we can build higher level constructs (like awareness of ntsc) |
| 20:01.13 | zero_level | is lowlevel == hidden ? |
| 20:01.41 | brlcad | it's hidden in the sense that it's all contained within the icv struct |
| 20:01.55 | zero_level | hmm ? |
| 20:02.05 | brlcad | but you'd tell ICV to do those operation on a given image |
| 20:02.23 | brlcad | img = icv_create() or icv_open() right? |
| 20:02.33 | zero_level | yes |
| 20:03.29 | brlcad | say I have an application that creates a 1-channel image, then need to convert it to "rgb" 3-channel |
| 20:03.48 | brlcad | so I use the API to create two more channels, replicating the data from the first/initial channel |
| 20:04.33 | zero_level | brlcad : api wise we could do n number of things. |
| 20:04.56 | Notify | 03BRL-CAD:indianlarry * 57437 brlcad/branches/nurbs/src/librt/primitives/brep/brep.cpp: breakout UV interval min/max distance checks, add bailout when first order walk vector less than tol, misc debugging code still WIP |
| 20:05.04 | zero_level | but If i were me. I would ask does itmake sense to have a 2-channel image ? |
| 20:05.11 | zero_level | no! |
| 20:05.22 | zero_level | It makes sense for a 3 channel image. |
| 20:05.28 | brlcad | but it does generalize the notion of 3-channel, 4-channel, and 1-channel images |
| 20:05.44 | brlcad | who is to say that 3 channels are rgb data? |
| 20:05.59 | zero_level | see it will hardly take 20 mins implementing such an api. |
| 20:06.03 | brlcad | datawise, there's a scaling function that might indicate that, might not |
| 20:06.08 | zero_level | but do we really want thatn ? |
| 20:06.48 | brlcad | you're already implementing that, it's just whether you spend time to implement a subset of the NxM possibilities (of which you already have a half dozen?) |
| 20:07.06 | brlcad | or you implement the four needed to generalize it |
| 20:07.25 | zero_level | lets hold it for the moment. |
| 20:07.45 | zero_level | I think developing a full fledged icv is a long process. |
| 20:07.58 | brlcad | to me, that's more important than getting N formats integrated |
| 20:08.03 | zero_level | at least 10 weeks more after GSOC. |
| 20:08.19 | brlcad | I don't think it's anywhere near that much work -- you already have most of it done |
| 20:08.41 | zero_level | no. we can do a lot. |
| 20:08.43 | brlcad | the logic for averaging N channels is already there |
| 20:08.49 | zero_level | for instance the image formats. |
| 20:08.56 | zero_level | OpenXR integration. |
| 20:09.26 | zero_level | Then We are yet to modify rt to send double datat. |
| 20:10.03 | brlcad | that's all nice, but doesn't buy me anything |
| 20:10.23 | brlcad | getting a handle on what we already do in a more maintainable fashion buys me a LOT |
| 20:10.26 | brlcad | if i'm going to have to completely rewrite every format because the base representation changed, I've got a big problem |
| 20:11.39 | brlcad | having to unwire a hundred or 400 or 20 tools that were made to call icv_rgb2uchar() instead of icv_average()+icv_reduce() (just an example) |
| 20:12.00 | brlcad | especially once there are several other functions that are in exactly the same position |
| 20:12.41 | zero_level | well. lets do it case by case. |
| 20:14.20 | zero_level | can you point to any other usage of icv_reduce. |
| 20:14.41 | zero_level | because it just not make sense to generalize for the sake of using it once. |
| 20:14.50 | zero_level | or twice. |
| 20:15.12 | zero_level | ^does |
| 20:15.37 | brlcad | I think we already have a case with rgb2gray as it 1) assumes a three-channel image represents red green and blue pixel data, 2) assumes I want a 1-channel result, 3) introduces colorspaces names that overlap somewhat with file format names |
| 20:16.46 | zero_level | ? |
| 20:16.50 | zero_level | so ? |
| 20:16.52 | brlcad | I think icv_rgb2gray_ntsc() and icv_rgb2gray_crt() belong up in application land, so you'd have N callers right there |
| 20:16.55 | zero_level | :) |
| 20:17.26 | zero_level | lets bring in more people. |
| 20:17.37 | zero_level | ``Erik : Are you available ? |
| 20:17.44 | brlcad | it took us how many hours just to get this far in understanding |
| 20:18.12 | zero_level | I think we are complicating a simple issue. |
| 20:18.24 | zero_level | And I am sorry for being reluctant |
| 20:18.28 | brlcad | I think you are making this sound a whole lot more complicated than it really is :) |
| 20:18.47 | brlcad | so lets put it on a back burner for later, there are other issues to address still |
| 20:18.50 | zero_level | ok. |
| 20:18.58 | zero_level | So what do we conclude ? |
| 20:19.01 | zero_level | :) |
| 20:19.19 | brlcad | put a note in TODO to review/evaluate how the API needs to accommodate channel and color space changes |
| 20:19.45 | zero_level | because I think on part of maintainability. |
| 20:20.10 | zero_level | the current structures gives us an edge. |
| 20:20.18 | zero_level | an the issue of generalization. |
| 20:20.19 | brlcad | I'm squarely concerned with maintainability |
| 20:20.42 | brlcad | that's why I'm for more generalization, not less, so that this will get used and not have longer term costs |
| 20:21.07 | zero_level | I dont think someonw will be interested to produce a 2 channel image ? :) |
| 20:21.11 | brlcad | don't care if there's a short term cost to make it slightly more general, even if it's only used in a couple places in our code if it means the concepts are trivial to understand |
| 20:21.41 | brlcad | like I said, the problem already exists with 1/3/4 channel images |
| 20:21.53 | brlcad | so 3 times however many conversion functions |
| 20:21.57 | brlcad | vs 4 functions |
| 20:22.03 | zero_level | and the problem is ? |
| 20:22.23 | brlcad | it only takes two conversion functions for it to be worthwhile to implement a generalization |
| 20:22.32 | brlcad | hsv+rgb, there ya go |
| 20:22.45 | brlcad | or hsv+gray |
| 20:23.33 | brlcad | zero_level: so part of the distinction here is a different perspective |
| 20:23.40 | brlcad | the tools in src/util are not just for image processing |
| 20:23.46 | brlcad | they're for *data* processing |
| 20:23.49 | brlcad | signals |
| 20:23.52 | brlcad | wavelengths |
| 20:24.06 | brlcad | that's why so many of them don't even care how many channels, they just take streams of bytes |
| 20:25.47 | brlcad | we have a multispectral library that lets us render images in practically any "color space segmentation", where it literally keeps track of an unlimited number of color "channels" for accurate spectral simulation |
| 20:25.49 | zero_level | is glad we are having this discussion |
| 20:26.18 | zero_level | ok. |
| 20:26.41 | brlcad | if you think of "red" and "green" and "blue" as being a range of wavelengths, it becomes clear that there are an infinite range of other possibilities that we could attach equipment and process |
| 20:27.19 | brlcad | image conversion is the simplest way to explain things, but it's all ultimately data drive by some (unknown) application |
| 20:27.45 | brlcad | I'd argue that ICV shouldn't be aware of "red" or "grayscale" |
| 20:28.07 | brlcad | it's merely aware of file formats and getting channel(s) of data into those formats |
| 20:28.48 | brlcad | that makes this an incredibly powerful library |
| 20:29.26 | brlcad | one that affects more than 100+ tools in BRL-CAD, some very well-suited for scientific computing |
| 20:29.40 | brlcad | is done :) |
| 20:31.31 | zero_level | brlcad : I see your point. |
| 20:31.47 | zero_level | thanks for your time. |
| 20:31.52 | brlcad | I lied saying I'm done -- one more point is just to say that I don't think this changes the SCOPE of ICV at all.. just the terminology used and how functions are grouped |
| 20:32.11 | zero_level | ok. |
| 20:32.18 | brlcad | still to the immediate timeframe, lets put bullet #3 on hold :) |
| 20:32.24 | brlcad | just a TODO to revisit |
| 20:32.26 | zero_level | ok. |
| 20:32.34 | zero_level | ok. |
| 20:32.49 | zero_level | wanders. |
| 20:33.57 | brlcad | zero_level: for your log when you return, icv_crop() looks just fine to me -- the docs are very clear on all those parameters. Might consider dropping the last two since that's really what icv_rect() does. |
| 20:34.35 | brlcad | zero_level: we can continue friday, but hopefully make some progress before then, no? |
| 20:34.42 | brlcad | or are you not working tomorrow? |
| 20:35.27 | zero_level | I am wrtting an exam tommorow (thursday). |
| 20:35.31 | brlcad | ah, okay |
| 20:35.35 | brlcad | have fun :) |
| 20:35.57 | brlcad | well, lets continue this discussion asynchronously |
| 20:35.57 | zero_level | yeah its related to organizational behaviour. |
| 20:36.11 | zero_level | so have learnt some lesson today. |
| 20:36.14 | brlcad | I'll reply to a few more and you can reply whenever you can -- I read the entire log |
| 20:36.32 | brlcad | and if we both happen to be on at the same time, we can get into detail |
| 20:36.39 | zero_level | alright. |
| 20:36.46 | zero_level | that is fine. |
| 20:36.51 | brlcad | i'll just prefix with zero_level: ;) |
| 20:38.41 | zero_level | also i have created a new paste forever. http://paste.kde.org/p38e0decb/ :) |
| 20:38.52 | brlcad | zero_level: #5 is more complicated than your understanding I think and gets at the heart of ICV's internal representation being double [0,1]. dpix is an arbitrary range of values (basically [-MAX_DOUBLE,MAX_DOUBLE] possibilities, some range therein) |
| 20:40.17 | brlcad | zero_level: for #5, ICV's internal representation will either need to change or become "lossy" when dealing with our own dpix format (and possibly openexr's format) -- if it's fully encapsulated, it shouldn't matter if it changes ;) |
| 20:41.07 | brlcad | zero_level: #6 uhm, YES, heh.. |
| 20:44.48 | brlcad | zero_level: #7: not loving the flags. implies I will have icv images that are potentially invalid, which would be fragile API. I think the data should always be "sanitized", and that function (icv_sanitize) should be hidden as it's not an "operation" per se but backend cleanup if it's going to remain |
| 20:48.05 | brlcad | zero_level: #8 rot.c is a mess as just a copy of pixrot.c, but an API call to "rotate" data certainly sounds important/useful; data-wise that's right up there with scaling data, translating data, truncating, replicating, and interpolating |
| 21:12.39 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 21:12.47 | *** join/#brlcad mpictor (~mark@2601:d:b280:3d4:d63d:7eff:fe2d:2505) |
| 22:10.43 | *** join/#brlcad FLOSSrookie (~brian@107-200-34-111.lightspeed.tulsok.sbcglobal.net) |
| 22:24.19 | Notify | 03BRL-CAD:starseeker * 57438 brlcad/trunk/src/conv/step/g-step/ON_Brep.cpp: Take a stab at arc curve conversion. |
| 23:56.58 | mpictor | n_reed: mind taking a look at a parser problem in stepcode? |
| 23:57.41 | mpictor | If I run https://github.com/stepcode/stepcode/blob/mp/bound_spec_reorder/src/exppp/test/exppp_prob_func.exp through exppp, line 10 becomes "res := [lis[1],5];" |
| 23:59.36 | mpictor | I need the bound_spec parser rule to be chosen over the aggregate_initializer rule |