What is a minor error?? Incomplete logic or functionality is fine, not an error. Submitting code with syntax errors would be untestable, not worth reviewing. Code with style errors reduces readability, is a burden on all other devs, and hurts maintainability.
If you know something is wrong, you should remove it, document it, or fix it. Ignoring is not responsible.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
:)))
I asked Shubham on IRC to upload his (buggy) code such that I can review it and see if he is going into the right direction. Then I discussed my fidings with him on IRC. Unfortunately the logs on rikers.org are incomplete.
However, he wants to come up with a fixed patch soon.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
From what I can see from here:
You should add the matrices to tk_vars too. Whith these casts it looks very dangerous to have it different from x_vars.
And in addition, xmat should be initialized in tk_open_dm().
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I've applied your patch but you should look after the following issues:
The BN_VLIST_SET_MODEL_MAT/RT_VLIST_SET_MODEL_MAT have no need for a point parameter.
bn_vlist_bbox(): At least the reference point from BN_VLIST_DISPLAY_MAT should be honored here, and it could be wise to ignore the points while in display_mat mode because they aren't in the model coordinate system.
in ~vars (display manager): Changing "matt xmat" to "fastft xmat" could eventiually ease the access (dereferencing) to the current matrix.
in tk_drawVList(): BN_VLIST_DISPLAY_MAT: Multiply the point with mod_mat and then replace the corresponding entries in disp_mat with it.
In rt_annot_plot() switch only once to display_mat and model_mat. E.g., in ant_to_vlist() could be one RT_VLIST_SET_DISP_MAT before the calls of seg_to_vlist() and one RT_VLIST_SET_MODEL_MAT after them.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
All your points have been worked upon.
The change in CMakelists is intentional as I was facing "-Werror,-Wnewline-eof" error on macos while building the project.
regarding RT_VLIST_SET ..., just wanted to be on the safe side to make sure everything gets initialized again. I guess there is no need for that so I'll change it in the next patch.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I applied your patch althogh the in command crashes mged. But, this shouldn't influence the rest of the program and it makes it easier for me to track your progress.
There are two memory issues with your code:
~ Memory management
~ Working with the coordinates
Here are my findings in detail:
In BN_VLIST_SET_MODEL_MAT: Why did you wrote _vp->pt[_vp->nused][0] = 0; there?
bn_vlist_bbox(): It could be wise to ignore the points while in display_mat mode because they aren't in the model coordinate system. (This one has a low priority.)
in ~vars (display manager): Changing "matt xmat" to "fastft xmat" could eventiually ease the access (dereferencing) to the current matrix. (low priority too)
in X_drawVList(), tk_drawVList(): BN_VLIST_DISPLAY_MAT: Replace the corresponding entries in disp_mat with the computed point because this is the point where now the (0, 0) should go to.
Adapt the other display managers too, when the issies with X and Tk are solved.
annot_in(): anip->vert_count has to be 2. Now, e.g., the allocated 2D point array is to small. (There could be more memory issues.)
seg_to_vlist(): What's this? V2ADD2 overwrites pt:
VMOVE(pt, V);
V2ADD2(pt, V, annot_ip->verts[tsg->ref_pt]);
The V parameter should be (0, 0, 0) or, better, not existend. The placement of the vlist elements does the display matrix in the display managers.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
All the above issues worked upon
Line segment also included (still it needs a stronger definition related to placement)
The coordinates are directly mapped onto the x-y plane
mat_t -> fastf_t
My findings:
In src/libbn/vlist.c: Where do you set flag? (BTW, "flag" is a bad name. If somebody reads the code this doesn't explain its purpose.)
The transformation matrix seems to have another format in OpenGl than usual. See e.g. ogl_loadMatrix(). Please, consider this when you handle this matrix.
I hesitate to accept your patch because it doesn't look well tested. Please, test your changes first before you upload them. And yes, I know that's not trivial.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
applied this patch with revision 70096 except src/libdm/dm-rtgl.c
What was your intention for dm-rtgl.c?
In src/libdm/dm-plot.c and dm-ps.c you haven't taken into account that plotmat and psmat aren't references. I corrected this in revision 70097 because otherwise these two would be broken.
Open issues:
BN_VLIST_DISP_MODE_ON/OFF shouldn't be in a public header because they aren't part of a public interface. In fact, I would simply use 0 and 1.
In src/libdm/dm-X.c (and dm-tk.c?) you have a lot of unnecessary braces around xmat.
In seg_to_vlist() the V parameter should be (0, 0, 0) (or better not existend).
And the scaling ...
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Hmm, the scaling won't work this way (try redrawing e.g. by zooming in and out). However, I committed your changes and I've an idea for the scaling too.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The patch looks formally good so far. Maybe, you should comment the new entries in include/bn/vlist.h.
I noticed, that the annot element doesn't memorize the label. Are you aeare of this?
[REVISED]
added comments for the new entries
[REVISED]
Please ignore the minor erros
What is a minor error?? Incomplete logic or functionality is fine, not an error. Submitting code with syntax errors would be untestable, not worth reviewing. Code with style errors reduces readability, is a burden on all other devs, and hurts maintainability.
If you know something is wrong, you should remove it, document it, or fix it. Ignoring is not responsible.
:)))
I asked Shubham on IRC to upload his (buggy) code such that I can review it and see if he is going into the right direction. Then I discussed my fidings with him on IRC. Unfortunately the logs on rikers.org are incomplete.
However, he wants to come up with a fixed patch soon.
Ah, then carry on! Showing progress is definitely better than working in obscurity. :)
[REVISED]
{BUILD PASSED}
From what I can see from here:
You should add the matrices to tk_vars too. Whith these casts it looks very dangerous to have it different from x_vars.
And in addition, xmat should be initialized in tk_open_dm().
[REVISED]
I've applied your patch but you should look after the following issues:
The BN_VLIST_SET_MODEL_MAT/RT_VLIST_SET_MODEL_MAT have no need for a point parameter.
bn_vlist_bbox(): At least the reference point from BN_VLIST_DISPLAY_MAT should be honored here, and it could be wise to ignore the points while in display_mat mode because they aren't in the model coordinate system.
in ~vars (display manager): Changing "matt xmat" to "fastft xmat" could eventiually ease the access (dereferencing) to the current matrix.
in tk_drawVList(): BN_VLIST_DISPLAY_MAT: Multiply the point with mod_mat and then replace the corresponding entries in disp_mat with it.
In rt_annot_plot() switch only once to display_mat and model_mat. E.g., in ant_to_vlist() could be one RT_VLIST_SET_DISP_MAT before the calls of seg_to_vlist() and one RT_VLIST_SET_MODEL_MAT after them.
All your points have been worked upon.
The change in CMakelists is intentional as I was facing "-Werror,-Wnewline-eof" error on macos while building the project.
Not really all points, but OK.
The change to CMakeLists.txt looks reasonable.
But, why don't you have one RT_VLIST_SET_DISP_MAT before the for-loop and one RT_VLIST_SET_MODEL_MAT after it?
regarding RT_VLIST_SET ..., just wanted to be on the safe side to make sure everything gets initialized again. I guess there is no need for that so I'll change it in the next patch.
[UPDATED]
Last edit: Shubham Rathore 2017-07-26
I applied your patch althogh the in command crashes mged. But, this shouldn't influence the rest of the program and it makes it easier for me to track your progress.
There are two memory issues with your code:
~ Memory management
~ Working with the coordinates
Here are my findings in detail:
In BN_VLIST_SET_MODEL_MAT: Why did you wrote _vp->pt[_vp->nused][0] = 0; there?
bn_vlist_bbox(): It could be wise to ignore the points while in display_mat mode because they aren't in the model coordinate system. (This one has a low priority.)
in ~vars (display manager): Changing "matt xmat" to "fastft xmat" could eventiually ease the access (dereferencing) to the current matrix. (low priority too)
in X_drawVList(), tk_drawVList(): BN_VLIST_DISPLAY_MAT: Replace the corresponding entries in disp_mat with the computed point because this is the point where now the (0, 0) should go to.
Adapt the other display managers too, when the issies with X and Tk are solved.
annot_in(): anip->vert_count has to be 2. Now, e.g., the allocated 2D point array is to small. (There could be more memory issues.)
seg_to_vlist(): What's this? V2ADD2 overwrites pt:
VMOVE(pt, V);
V2ADD2(pt, V, annot_ip->verts[tsg->ref_pt]);
The V parameter should be (0, 0, 0) or, better, not existend. The placement of the vlist elements does the display matrix in the display managers.
All the above issues worked upon
Line segment also included (still it needs a stronger definition related to placement)
The coordinates are directly mapped onto the x-y plane
mat_t -> fastf_t
[REVISED]
rt_annot_ifree included
[REVISED]
My findings:
In src/libbn/vlist.c: Where do you set flag? (BTW, "flag" is a bad name. If somebody reads the code this doesn't explain its purpose.)
The transformation matrix seems to have another format in OpenGl than usual. See e.g. ogl_loadMatrix(). Please, consider this when you handle this matrix.
I hesitate to accept your patch because it doesn't look well tested. Please, test your changes first before you upload them. And yes, I know that's not trivial.
[REVISED]
applied this patch with revision 70096 except src/libdm/dm-rtgl.c
What was your intention for dm-rtgl.c?
In src/libdm/dm-plot.c and dm-ps.c you haven't taken into account that plotmat and psmat aren't references. I corrected this in revision 70097 because otherwise these two would be broken.
Open issues:
I've tried using a factor for the scaling, it works on my system. Please have a look. Plus other changes for the corresponding issues are there too
Hmm, the scaling won't work this way (try redrawing e.g. by zooming in and out). However, I committed your changes and I've an idea for the scaling too.