Feature: Add getters for camera parameters #1419 #1663
Conversation
|
You are modifying libf3d public API! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1663 +/- ##
==========================================
- Coverage 95.81% 95.80% -0.01%
==========================================
Files 125 125
Lines 10530 10555 +25
==========================================
+ Hits 10089 10112 +23
- Misses 441 443 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| double viewDirProj[2] = { viewDir[0], viewDir[2] }; | ||
| if (vtkMath::Dot2D(viewDirProj, viewDirProj) < VTK_DBL_EPSILON) | ||
| { | ||
| return 0.0; |
There was a problem hiding this comment.
please add a test for this
There was a problem hiding this comment.
in short you need to add a test where this conditionj is true and you enter this line
|
@Schwarzemann any news on this ? |
Ah yes. Sorry work has been very busy lately. I will take care of it this week for sure. |
|
Do you need any help moving forward ? |
Actually yes kind of. I would really appreciate it. I just can't wrap my head around the tests also can't seem to include |
|
Do you need any help moving forward ? |
Not right now thanks. I just have problems with time management nowadays. |
Schwarzemann
changed the title
|
Do you need any help moving forward @Schwarzemann ? |
|
Do you need any help moving forward ? |
Sorry. I was supposed to push my latest changes but I haven't yet I will push as soon as possible and let you know. |
|
Need any help @Schwarzemann ? |
|
Are you around ? I can help and guide if needed :) |
Hello. Sorry I've been very busy lately. Yes I would greatly appreciate the help. I am still having problems with the test. Everything else seems fine. |
library/public/camera.h
Outdated
| /** Rotate the camera about its horizontal axis, centered at the focal point. */ | ||
| virtual camera& elevation(angle_deg_t angle) = 0; | ||
| virtual camera& addElevation(angle_deg_t angle) = 0; |
There was a problem hiding this comment.
While I agree that it makes more sense to change the name of these methods, if you do that you need to ensure retro compatibility of the API and deprecate the previous methods.
I'd suggest not to do that for now, I can take care of it.
There was a problem hiding this comment.
Should I rename them? If yes I can take care of that.
There was a problem hiding this comment.
yes, you should change the name compared to master
.vscode/settings.json
Outdated
| "files.associations": { | ||
| "cmath": "cpp" | ||
| } | ||
| } |
library/testing/TestSDKCamera.cxx
Outdated
| std::cerr << std::setprecision(12) << "azimuth: " << azimuth << " != 90.0" << std::endl; | ||
| return EXIT_FAILURE; | ||
| } | ||
| double viewDirProj[2] = { 1.0, 1.0 }; |
There was a problem hiding this comment.
| double viewDirProj[2] = { 1.0, 1.0 }; | |
| const double viewDirProj[2] = { 1.0, 1.0 }; |
I've left some comments. The main point is that we should not change the API. I can take care of that if we want to but for now lets just add the getters. |
Okay yes most certainly I will take care of it tonight. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
python/testing/test_camera.py
Outdated
| @@ -106,4 +106,4 @@ def test_resets(): | |||
| camera = engine.window.camera | |||
| camera.set_current_as_default() | |||
| camera.reset_to_bounds() | |||
| camera.reset_to_default() | |||
| camera.reset_to_default() | |||
There was a problem hiding this comment.
| camera.reset_to_default() | |
| camera.reset_to_default() |
python/testing/test_camera.py
Outdated
| camera.reset_to_default() | ||
|
|
There was a problem hiding this comment.
Should I delete the line?
There was a problem hiding this comment.
not exactly, you should fix the "missing end of line" issue. See this:
https://stackoverflow.com/questions/5813311/whats-the-significance-of-the-no-newline-at-end-of-file-log
Opening a new PR.