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

Draft: Dimension styles #3167

Merged
merged 20 commits into from Apr 16, 2020
Merged

Conversation

carlopav
Copy link
Contributor

@carlopav carlopav commented Mar 13, 2020

This PR provide objects to store annotation styles inside the document, and modify the annotation objects to store the link to their stile.

To be merged after PR #3102

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written
  • Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

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.19 Changelog Forum Thread.


@wwmayer wwmayer changed the title Dimension styles Draft: Dimension styles Mar 14, 2020
"""
Apply the style to the related dimensions
"""
from draftutils import gui_utils
Copy link
Contributor

Choose a reason for hiding this comment

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

This import should be at the top of the file, after draftutils.utils, not inside the function.

Comment on lines 99 to 100
super().__init__()
self.command_name = "DimensionStyle"
Copy link
Contributor

@vocx-fc vocx-fc Mar 21, 2020

Choose a reason for hiding this comment

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

This should be

def __init__(self):
    super().__init__(name="Dimension style")

The parent class, GuiCommandSimplest, sets up the command_name attribute.



def execute(self, obj):
import DraftGeomUtils
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be imported at the top of the file.

class LinearDimension(DimensionBase):
"""The Draft Dimension object"""
def __init__(self, obj):
DimensionBase.__init__(self,obj,"Dimension")
Copy link
Contributor

Choose a reason for hiding this comment

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

The better way is to call super().__init__(self,obj,"Dimension").

We prefer to avoid repeating the parent class in case it changes.

from PySide.QtCore import QT_TRANSLATE_NOOP

if App.GuiUp:
import FreeCADGui as Gui
from draftviewproviders.view_style_dimension import ViewProviderDraftDimensionStyle
from draftviewproviders.view_dimensionstyle import ViewProviderDraftDimensionStyle
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to guard the view providers because they won't be executed by the import, so there is no risk of causing an error. I mean, it doesn't hurt at all to guard them like this, I just think it looks a bit ugly.

We have to guard FreeCADGui because if we don't we will definitely cause an error in console mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

About this, I think some Travis tests are failing when we import the view provider module because inside this module FreeCADGui is not guarded.

So inside view_dimensionstyle.py and all those new view providers.

if App.GuiUp:
    import FreeCADGui as Gui

self.node3d.removeChild(self.marks)

def draw_dim_arrows(self, vobj):
from pivy import coin
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be importing coin inside functions. These should be at the top.

_Dimension = LinearDimension

from draftviewproviders.view_dimension import ViewProviderLinearDimension
_ViewProviderDimension = ViewProviderLinearDimension

return obj
Copy link
Contributor

Choose a reason for hiding this comment

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

@carlopav You forgot to remove this return obj statement, and when the entire Draft.py is imported, it causes a syntax error. The Travis tests show this and fail because of it.

@carlopav carlopav force-pushed the DimensionStyles branch 2 times, most recently from f913716 to eeb22b5 Compare March 22, 2020 08:40
@vocx-fc
Copy link
Contributor

vocx-fc commented Mar 23, 2020

Just another note, the Travis tests fail because when other workbenches import Draft.py, the draftobjects.dimension module is not found. The problem is that this module was not added to the CMakeLists.txt file. So, the new files dimension.py, view_dimension.py, etc. must be added to the appropriate variable in CMakeLists.txt, then they will be installed correctly in Travis, and hopefully the tests will pass.

elif p3 == "diameter":
#l.append((p1,"Diameter"))
if App.GuiUp:
obj.ViewObject.Override = "Ø $dim"
Copy link
Contributor

Choose a reason for hiding this comment

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

The Travis tests are failing in this line because of the unicode character "Ø $dim" . You have to add the codification at the beginning of the file. # -*- coding: utf8 -*-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for looking into this. I'll add it right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't I add it to every file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add it to every file, only to the one reported by our tests. This is actually only required in Python 2. But if it causes this error in Travis, it may mask other errors elsewhere, so we try to have no errors at all when making a pull request.

@carlopav carlopav force-pushed the DimensionStyles branch 3 times, most recently from b5e6a55 to 3502877 Compare March 23, 2020 23:04
elif obj.LabelType == "Area":
if hasattr(obj.Target[0],'Shape'):
if hasattr(obj.Target[0].Shape,"Area"):
obj.Text = [App.Units.Quantity(obj.Target[0].Shape.Area,App.Units.Area).UserString.replace("^2","²")]
Copy link
Contributor

Choose a reason for hiding this comment

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

This small 2 is also causing codification problems. # -*- coding: utf8 -*-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thx for spotting it!

@carlopav
Copy link
Contributor Author

@vocx-fc any idea why the build fail on linux? (if I can read it correctly)
https://travis-ci.org/github/FreeCAD/FreeCAD/builds/666987452?utm_medium=notification&utm_source=github_status

from draftviewproviders.view_dimension import ViewProviderAngularDimension
_ViewProviderDimension = ViewProviderLinearDimension

_ViewProviderAngularDimension = ViewProviderAngularDimension
Copy link
Contributor

Choose a reason for hiding this comment

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

In Travis the gui doesn't exist. So this view provider also has to be inside the if gui: so it is skipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

and made it derived from 
DimensionBase <- Draft Annotation
And based them on Draft Annotation object
Further cleanup and guarded imports of Gui in modules.

.
Dimension style property is auto-set on dimension creation.
Further improvementes in DimensionStyleContainer.
.
Removed Annotation styles current implementation.
As pointed out by yorik, in https://forum.freecadweb.org/viewtopic.php?f=23&t=44051&p=385710#p385179, this feature will be added using document Meta property.
- corrected super() methods to be Py2 compatible
- further cleanup of the code.
further cleanup


changed again to avoid super method


updated super() functions


updated to correct the parent classess targeted by super()
@carlopav
Copy link
Contributor Author

carlopav commented Apr 16, 2020

@vocx-fc thanks! it worked. (i didn't realize also the base object for bim assembly experimentations that realthunder provided had the (object) in the class)
@yorikvanhavre here we are!

@yorikvanhavre yorikvanhavre merged commit 131961c into FreeCAD:master Apr 16, 2020
@yorikvanhavre
Copy link
Member

Victory!!

@carlopav
Copy link
Contributor Author

Wow! A big thank you to everyone! :D

@carlopav carlopav deleted the DimensionStyles branch April 16, 2020 10:19
@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 16, 2020

I just noticed that the image files Draft_Annotation_Style.svg, Draft_Dimension_Style.svg, and Draft_Dimension_Style_Tree.svg aren't shown in GitHub. It says that the files are invalid. This probably means that the files have some non-standard Inskcape tags. So, when you save icons, make sure they are saved as Plain SVG.

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 16, 2020

Also, I noticed that the GuiCommandSimplest and GuiCommandNeedsSelection classes in gui_base.py were added twice.

Once they were added in this branch (530e8b6), and the second time they were added in my branch #3182 (017226e). Obviously this is not ideal, so I rebased my branch #3291 and added a new commit to remove the repeated classes.

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 16, 2020

Also, I integrated the small change made to the Draft_Label tool, in ff3bdf8, to my branch #3299.

@carlopav
Copy link
Contributor Author

My bad, those Icons (Draft_Annotation_Style.svg, Draft_Dimension_Style.svg, Draft_Dimension_Style_Tree.svg) are obsolete, i was sure I had removed them...
They were used for the DimensionStyle object that we decided to discard.
I'll make another PR to remove them. Thanks also for the other things spotted :)

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 16, 2020

About the icons, it seems you only added one, however, I checked your partial commits and that's when I noticed the error displaying all of them. Nevertheless, I'm unsure if this is really a problem from your side. Maybe something is wrong with GitHub at the moment because I can't see any icons.

For example, #3060 introduced many icons and they were visible without problems then, but now they aren't displayed. I've read that GitHub has been having a few issues the last few weeks, so that may be a cause.

@carlopav
Copy link
Contributor Author

Yes, just one of the icons is still present in the code. I think I just forget about it... I'll delete it during the weekend!

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 17, 2020

Just one more thing Carlo. Can you check why the size of the text is now different? Use the test script.

import drafttests.draft_test_objects as dt
dt.create_test_file()

Before your changes, all text would fit correctly in the rectangle, now it's too big. Maybe the sizes need to be set to a different value in the test script.

@carlopav
Copy link
Contributor Author

carlopav commented Apr 17, 2020

I think this is because of the scale multiplier property: when you create a dimension it automatically takes the ScaleMultiplier property according to the statusbar scale.
If you set this property to 1, they should look good. Also if you set the scale 1:1 in the statusbar, they will be created correctly.
@yorikvanhavre Anyway I spotted a bug and PR the fix: #3357

EDIT: you remember me that I have to update the wiki

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 17, 2020

Yes, I figured that much, the issue is the default scale multiplier is too large. In that case, you should set the scale multiplier to 1 by default to keep the same presentation as before. Otherwise new users will see their texts changed and won't know the reason.

@carlopav
Copy link
Contributor Author

well, not exactly: this affect only new created objects.
So if the user had them from before, or close and open the document, they will be displayed correctly.
The problem with the test is that it creates new objects...perhaps we can modify the make_dimension and make_text function to accept also this parameter... ?

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 17, 2020

Yes, but why should the default scale be anything but 1? That's more or less the issue. The test script is supposed to produce the same result every time it is run; if it doesn't it means something is changing the behavior. In this case, I think the natural solution is to set the scale to 1 by default, not to modify the make functions. Of course, the make functions can also be improved, but that's a separate discussion.

@carlopav
Copy link
Contributor Author

carlopav commented Apr 17, 2020

The default is in fact set to 1, but it is stored inside FreeCAD parameters:

param = App.ParamGet("User parameter:BaseApp/Preferences/Mod/Draft")

annotation_scale = param.GetFloat("DraftAnnotationScale", 1.0)

if annotation_scale != 0:

vobj.ScaleMultiplier = 1 / annotation_scale

this is the code involved.
The point is that you did set it to something else last time, it keeps the different scale, as all other parameters do in Draft Annotation since before I started editing them.
So, this is a good point. Are those supposed to be Document parameters or General parameters? Is this the right question to ask?

@vocx-fc
Copy link
Contributor

vocx-fc commented Apr 17, 2020

I set it to something else myself? Huh? Well, I wouldn't know. Maybe at some point I tested your branch and that value was written at that point.

I think keeping them as Draft parameters is fine.

This is a similar discussion as with your annotation styles concept. If you move the file to another system, it should display the same, and not change the text arbitrarily because of the different setup. This works correctly right now, because you save the scale as a property, so it should work fine. As long as only new objects are affected, there shouldn't be a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants