Sketcher: Infinite axes #12319
Sketcher: Infinite axes #12319
Conversation
| @@ -3295,6 3295,27 @@ void ViewProviderSketch::camSensCB(void* data, SoSensor*) | |||
| vp->onCameraChanged(cam); | |||
| } | |||
|
|
|||
| Base::Vector3d ViewProviderSketch::getCamCenterInSketchCoordinates(const Gui::View3DInventorViewer* viewer) const | |||
There was a problem hiding this comment.
This function should not be required. You should be able to access the one that the grid is using : GridExtensionP::getCamCenterInSketchCoordinates()
If you can't access this function, then move it to somewhere that makes sense so that you can access it. But I think you should be able to with pImpl->getCamCenterInSketchCoordinates()
As a bonus you won't need viewer if you use this.
There was a problem hiding this comment.
Actually I have just tried and you can't access it because pImpl is private. So you need to make a function in ViewProviderGridExtension to access it. Basically :
ViewProviderGridExtension:: getCamCenterInSketchCoordinates(){
return pImpl->getCamCenterInSketchCoordinates();
}
There was a problem hiding this comment.
Or the function can be moved to ViewProviderGridExtension, I don't know if pImpl has access to ViewProviderGridExtension.
There was a problem hiding this comment.
I wouldn't couple this with the ViewProviderGridExtension machinery as the latter, as the name implies, has to do with handling/drawing the grid.
The method name is also misleading as it's not really sketcher-specific.
Perhaps this function could be moved to a common module (where?) and be used by both the axis and the grid sizing mechanisms.
There was a problem hiding this comment.
Actually ViewProviderSketch is a ViewProviderGridExtension (derives from it). So there no reason you wouldn't access its functions.
Yes the name is not strictly ideal as the gris has been separated from VPSketch. You can rename it something like : getCamCenterIn2DObjectCoordinates.
There was a problem hiding this comment.
I see. Well it is indeed harder than it seems.
Then I think the function should be in core in viewInventor
There was a problem hiding this comment.
Ah but wait I'm dumb, I wrote exactly what you need in my assembly PR :
View3DInventorViewer::getPointOnXYPlaneOfPlacement
https://github.com/FreeCAD/FreeCAD/pull/10764/files
And this assembly PR is planed to merge very soon (it was to merge yesterday but had a conflict). So if you wait a few days you can then make a new function :
getCamCenterOnXYPlaneOfPlacement() that pass the cam cetner to getPointOnXYPlaneOfPlacement
There was a problem hiding this comment.
Awesome news, I'll wait 'till the PR is merged then.
There was a problem hiding this comment.
@LemonBoy 10764 merged yesterday. If you rebase you'll have access to this function.
There was a problem hiding this comment.
Thanks for the heads up. I took the chance to address another problem that I've also seen when using the grid, for some "weird" camera orientations the axis length was too short because it didn't take into account the perspective.
The new method seems to work fine, I'm now worried about the computational cost as the camera sensor callback runs continuously.
| Gui::View3DInventorViewer* viewer = view->getViewer(); | ||
|
|
||
| // Add 50% to be sure the whole viewport is covered. | ||
| const float halfAxisLength = 1.5f * viewer->getMaxDimension() / 2.f; |
There was a problem hiding this comment.
This is imo needless computation. Be 50 or 75% it doesn't change anything. So :
const float halfAxisLength = viewer->getMaxDimension();
Resize the axes according to the viewport size so that they appear as infinite.
for more information, see https://pre-commit.ci
Same result with less code.
When the sketch is rotated or shifted from the origin we need some further adjustments.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Find the correct size by projecting the viewport onto the sketch plane.
LemonBoy
force-pushed
the
sketcher-infinite-axes
branch
from
3f0f2a4
to
f5f6ba6
Compare
Attach the cameraSensor SoSensor to the actual camera node instead of the whole scene, avoids lots of spurious updates when the camera is not moving at all.
|
With the latest commit the redraw code is called less times and is actually correct even in presence of perspective distortion, see the image below to see how the grid boundaries stop before the viewport boundaries.
There's still some room for optimization, the plan is to create a specialized Abstracting the viewport projection to its own routine is also something I'd do as it may be useful for other features and to "fix" the computation of the grid size. As usual, suggestions are welcome. |
|
Fixes #8675 as well? @FlachyJoe |
|
@maxwxyz Yes! 😃 |
|
@LemonBoy please edit the PR to also include |
It turns out there was a missing piece, namely the final transform, to make the viewport projection work.
Attach the sensor to the camera to reduce the amount of calls to the camera update callback. Handle the camera object destruction in the best possible way given the few tools provided by Coin3D.
for more information, see https://pre-commit.ci
|
The latest changes fix the projection problems I've noticed and greatly reduces the axis recomputation. The graphical results are good in orthographic mode, in perspective mode there are still some problems that also affect the grid, don't know if we can consider that as a blocker for merging. The amount of times the sensor callback is invoked is much less than before. Please give this a try and report back if you see any problem. |
|
Any news on this, should this still be in draft or is this ready? |
|
@LemonBoy also a ping on this PR: |
|
Please consider this as a real bug which should be solved before 1.0 The reasoning: Think of making some sketch in full resolution and go on with adding something bigger outside by zooming out first. Tested with self compiled FreaCad as of 2024-07-27. |
Resize the axes according to the viewport size so that they appear as infinite.
The implementation is trivial, let me know if I've overlooked something.
The previous auto-resizing behaviour is now gone for good, the machinery to compute the sketch BB is still there so re-adding this option is easily done if needed.