-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
embrace trigonometry (fix also issue #164) #165
base: melodic-devel
Are you sure you want to change the base?
embrace trigonometry (fix also issue #164) #165
Conversation
In thoery, this modification is just about readability. It should give EXACTLY the same result, but... In practice, we fixed also a problem about mini/max distance, as reported in SteveMacenski#164
THis is emabrassing.... there is a compilation error. Let me fix it |
@tasilb can you test this? If you set |
Sure thing, though I might have to take my time with it due to hardware availability
…________________________________
From: Steve Macenski <notifications@github.com>
Sent: Tuesday, April 7, 2020 11:24:36 AM
To: SteveMacenski/spatio_temporal_voxel_layer <spatio_temporal_voxel_layer@noreply.github.com>
Cc: David Tsai <tcdavid372@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [SteveMacenski/spatio_temporal_voxel_layer] embrace trigonometry (fix also issue #164) (#165)
@tasilb<https://github.com/tasilb> can you test this? If you set VISUALIZE_FRUSTUM 1 in the depth camera frustum file, it'll publish the points of the frustum (kill performance, but you can see the frustum corners and normal vectors. Just making sure I don't break you guys -- stuff inside updating, stuff outside not, the usual.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#165 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABJYI5EW7UIR3UCQ2VLL4G3RLNVWJANCNFSM4MDBB5KA>.
|
@tasilb have you taken a look at this? |
Are there any updates on this? I've noticed a significant improvement in CPU juice after removing the normalize operation |
I guess the changes are still valid 😆 Using std array instead of vector surely help too |
@mlsdpk if you can validate that this represents the right frustum now, I can merge this in. |
Understood! I'll do my best to share results from real hardware, though I cannot guarantee it due to confidentiality concerns. Is it acceptable if I submit a PR for unit tests to compare the points from pcl::FrustumCulling filter with STVL's depth camera frustum? This would allow us to thoroughly test the changes introduced in this PR also. Please let me know if this approach aligns with your expectations. |
Visualizing the frustum and/or measuring the plane angles and/or measuring the corner points and comparing them to manually computed values from another method would be all ways to validate. Certainly pcl would be another way :-) Unit tests are always appreciated, but not required for this repository. This is largely in legacy maintenance mode with few/no outstanding issues (beyond this) that hopefully will be superseded by something building on this idea with probabilistic modeling in the medium term future. I usually require much more formality, but this is an old project that predated that policy and I expect that if Davide tested it, it probably works fine, I just want a 3rd opinion since we're all human |
In theory, this modification is just about readability. It should give EXACTLY the same result, but...
In practice, we fixed also a problem with min/max distance, as reported in #164
Please test it before merging it, to be sure I didn't miss anything.