Skip to content
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

Add Zoom Actual (1:1 zooming) command to View -> Zoom menu #5125

Closed
wants to merge 3 commits into from

Conversation

mwganson
Copy link
Contributor

@mwganson mwganson commented Oct 23, 2021

Based on a macro from mario52: (Edit: kisolre authored the macro)

https://forum.freecadweb.org/viewtopic.php?p=447004#p454617

Zooms out to a distance such that objects are shown on the screen at their actual size. For example, you can take a ruler and measure a standard Cube to be 10mm across on your screen after executing this command.

Notes:

On my Virtual Machine compile computer (Ubuntu running inside Virtual Box VM, Windows 10 Host) it does not work properly. The Cube measures 15mm approximately. But I think this is due to being in the VM it's not getting the correct physical hardware dimensions. Running the macro gives the same results on the VM, so I don't think it's an error in translation from macro code to the PR. Macro works properly on my Windows 10 host, but I can't test the build there. It needs to be tested.

My skills with Inkscape are limited at best, and that is being generous. The icon, while somewhat serviceable, definitely could use some work.

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, we ask you to conform to the following items. Pull requests which don't satisfy all the items below might be rejected. If you are in doubt with any of the items below, don't hesitate to ask for help in the FreeCAD forum!

  • Your pull request is confined strictly to a single module. That is, all the files changed by your pull request are either in App, Base, Gui or one of the Mod subfolders. If you need to make changes in several locations, make several pull requests and wait for the first one to be merged before submitting the next ones
  • In case your pull request does more than just fixing small bugs, make sure you discussed your ideas with other developers on the FreeCAD forum
  • Your branch is rebased on latest master git pull --rebase upstream master
  • All FreeCAD unit tests are confirmed to pass by running ./bin/FreeCAD --run-test 0
  • All commit messages are well-written ex: Fixes typo in Draft Move command text
  • Your pull request is well written and has a good description, and its title starts with the module name, ex: Draft: Fixed typos
  • Commit messages include issue #<id> or fixes #<id> where <id> is the FreeCAD bug tracker issue number in case a particular commit solves or is related to an existing issue on the tracker. Ex: Draft: fix typos - fixes #0004805

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.20 Changelog Forum Thread.


@mwganson
Copy link
Contributor Author

@luzpaz
Copy link
Contributor

luzpaz commented Oct 23, 2021

Added screenshot proposed feature (from the forum thread)

image

@luzpaz luzpaz added enhancement Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD labels Oct 24, 2021
@Syres916
Copy link
Contributor

Tested the command by copying it into ViewFitAll and recompiling, works as expected with a 200mm Cube on a 2560x1440 display using:
OS: Linux Mint 19.3 (X-Cinnamon/cinnamon)
Word size of FreeCAD: 64-bit
Version: 0.20.26201 (Git)
Build type: Release
Branch: master
Hash: 9f2dd4a
Python version: 3.6.9
Qt version: 5.9.5
Coin version: 4.0.0a
OCC version: 7.3.0
Locale: English/UnitedKingdom (en_GB)

@freecadci
Copy link

pipeline statusfor feature branch PR_5125. Pipeline #394344028 was triggered at fec172b. All CI branch pipelines.

Comment on lines 2504 to 2505
doCommand(Command::Gui,"Gui.ActiveDocument.ActiveView.getCameraNode().height\
.setValue(min(Gui.activeView().getSize()[0],Gui.activeView().getSize()[1])*25.4/Gui.getMainWindow().physicalDpiX())");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mwganson do you mind adding a comment here on what this equation does?

@mwganson
Copy link
Contributor Author

I'm going to add a user parameter, default 1.0, that users can use to tweak this if it's not working correctly on their hardware. Usage would be make a default cube of 10 mm, measure after executing zoom actual, divide your measurement by 10 mm and use that for the tweak. For example, if the 10 mm cube measures 15 mm on your ruler, then enter 15 / 10 = 1.5 as the tweak parameter.

@mwganson
Copy link
Contributor Author

I have added the tweak parameter and the icon file. I think that was why the ci had failed, no icon file. The reason for the forgotten file was the first PR (which I didn't submit) included some unrelated changes, so I deleted that branch and made a new one without the unrelated changes, and neglected to add the icon file in the new branch. Sorry about that. I should have rebuilt to test, but I didn't think I needed to.

Tweak parameter is in BaseApp/Preferences/View and is called ZoomActualTweak. It is a float value. Usage: if the object appears 1.5 x the expected size enter 1.5 as the tweak parameter. Default is 1.0. I was torn between entering as 1.5 or 1/1.5.

Question: Should the tweak parameter be in user preferences dialog? I don't think so because rare should be the case where it is needed. A compromise is to add it to user parameters so it is there already should the user need it.

@mwganson
Copy link
Contributor Author

User mario52 has corrected me. He was quoting user kisolre, author of original macro. Comments in code corrected.

@adrianinsaval
Copy link
Member

It should be documented in the fine tuning wiki page if it's just going to be a parameter

@freecadci
Copy link

pipeline statusfor feature branch PR_5125. Pipeline #394426909 was triggered at 8e60f96. All CI branch pipelines.

@yorikvanhavre
Copy link
Member

This relies on Qt to tell us the display size... Isn't that hazardous somehow? I don't know if I would trust Qt to give the correct result everywhere, always, on all platforms :) I wonder if it's a good idea to add this feature "by default", if it's going to give wrong results for a quantity of users (which I see you are foreseeing it to happen, as you introduced a parameter to fix it)...
Basically, isn't it better to keep as a macro, where users must specifically install it, and therefore are probably more knowing what they are doing?

@luzpaz
Copy link
Contributor

luzpaz commented Oct 25, 2021

yorik, perhaps the user can also have the ability to add the screen size parameters as an option?

@yorikvanhavre
Copy link
Member

yorik, perhaps the user can also have the ability to add the screen size parameters as an option?

Yes, that's the purpose of the ZoomActualTweak parameter. But it won't be clear for many users if you need to set it and how. Many might use this function and get wrong results, and, as you know, complain before looking into the docs :) So I'm wondering if we want the hassle to have to explain many, many times how to use this tool...

@adrianinsaval
Copy link
Member

Will it actually fail often? Maybe a warning in the tooltip or a mesage in the status bar when activated could give people the hint to this parameter. Just tested the python macro on windows (PC with a monitor) and it works. Will test on a laptop on windows and linux when I get the chance.

@mwganson
Copy link
Contributor Author

Another thing I noticed in testing is if the user sets the tweak parameter to 0 it locks up zooming -- can't zoom in or out anymore. I guess a camera height of infinity is asking too much. I had to restart FreeCAD, but I was able to restart in a manner that would have allowed me to save what I was working on. The fix for that would be simple enough, test the tweak parameter to ensure it's greater than 0 or else set it back to the default of 1.0 before using it and send a warning about invalid tweak parameter to the report view. I'll go ahead and add that to it shortly in case we decide to merge this.

…1 or else use default 1.0 and display error message in report view
@freecadci
Copy link

pipeline statusfor feature branch PR_5125. Pipeline #395380962 was triggered at 0571623. All CI branch pipelines.

@mwganson
Copy link
Contributor Author

mwganson commented Oct 27, 2021

I've opened a discussion for this and a 3-day poll so we can get a better idea maybe of what the failure rates are and go from there. If it fails too much, then we can just close this and be done. If no failures are reported and we get a big enough sample size, then I say merge.

https://forum.freecadweb.org/viewtopic.php?f=8&t=63258

@wwmayer
Copy link
Contributor

wwmayer commented Oct 28, 2021

Please note that this function will raise an error if the current camera is not orthographic.

If you e.g. set it to perspective this line
cam.height.setValue(invHeight*25.4/vpDpi)
raises an error that the SoPerspectiveCamera object has no attribute height

@freecadci
Copy link

pipeline statusfor feature branch PR_5125. Pipeline #398053401 was triggered at 5c1801a. All CI branch pipelines.

@mwganson
Copy link
Contributor Author

mwganson commented Nov 4, 2021

Closing. I agree with yorik. Qt cannot reliably give us the correct dpi / screen size.

@mwganson mwganson closed this Nov 4, 2021
@mwganson mwganson deleted the zoomactual branch December 10, 2021 06:03
@luzpaz luzpaz added the Feature FR for improvements or new features label Mar 4, 2022
@luzpaz luzpaz removed the enhancement label Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Issue or PR touches core sections (App, Gui, Base) of FreeCAD Feature FR for improvements or new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants