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

SchematicEditor: Fix net label offset after mirroring #451

Merged
merged 10 commits into from May 24, 2019

Conversation

@alfishe
Copy link
Contributor

commented May 12, 2019

Added offset for Net label after mirrorring that compensate inability for NetLabel to autoadjust after mirroring.
Assuming that it's correct operation to get NetLabel rendering boundary rectangle and after transformation to schematic page coordinates apply offset to each net label

Fix for issue #335 - SchematicEditor: Net labels are not mirrored pro…
…perly

Added offset for Net label after mirrorring that compensate inability for NetLabel to autoadjust after mirroring
@ubruhin
Copy link
Member

left a comment

Great, thanks! 😃

I added some comments (see below), and I will test the changes soon.

Btw, It would be nice if you could format the code with clang-format (see details here), if possible. But don't worry if you don't get it done, then I could do it by myself ;)

alfishe and others added some commits May 12, 2019

Update libs/librepcb/projecteditor/cmd/cmdmirrorselectedschematicitem…
…s.cpp

Co-Authored-By: U. Bruhin <urbibruhin@bluewin.ch>
Update libs/librepcb/project/schematics/items/si_netlabel.cpp
Co-Authored-By: U. Bruhin <urbibruhin@bluewin.ch>
Update libs/librepcb/project/schematics/items/si_netlabel.h
Co-Authored-By: U. Bruhin <urbibruhin@bluewin.ch>
@alfishe

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Well, proposing changes with incomplete and inconsistent result - not very friendly for contributors.

  1. Snapping changes to grid requires casting from PositiveLength to Length. Unable to figure out how to do that properly
  2. If you change method name in one place (definition) it needs to be changed in two other (declaration and call)

Please merge changes to your discretion. I see no sense to overcome problems that didn't exist in original pull request.

@ubruhin

This comment has been minimized.

Copy link
Member

commented May 13, 2019

Unfortunately GitHub Suggestions are not powerful enough to make perfect, fully functional code change suggestions:

  • One can only propose single line changes. It's not possible to make suggestions which change the count of lines. Thus I was not able to make a working suggestion for removing the local variable.
  • It's not possible to group related changes together. For example when renaming a function, I would need to create 3 independent suggestions (declaration/definition/usage) and you need to review 3 suggestions until you get the complete picture of the suggestion. On larger PRs, this leads to a lot of noise in reviews.
  • CI is (obviously) not run for the suggestions, so neither the author nor the committer of suggestions can know if CI will succeed after applying them.

After all, I consider this feature just as an alternative to explaining the suggestion in words (code is often easier to write and understand than text). In the end there is often no way to avoid implementing/improving/code-formatting/testing the suggestions locally, and IMHO it would be strange to do all these things before making a suggestion, just to be able to create perfect formatted and tested suggestions (it's still only a suggestion to the PR author, not a strong change request).

Sorry for the inconvenience because it was not clear in advance how my suggestions are meant 🙂

Snapping changes to grid requires casting from PositiveLength to Length. Unable to figure out how to do that properly

Oops, I didn't notice that 😱 The conversion can be made with the dereferencing operator *:

 newpos.setX(newpos.getX() - netlabel->getWidth().mappedToGrid(*mSchematic.getGridProperties().getInterval()));

Now I also tested your implementation. For horizontal labels it works as expected, but vertical labels (rotation +/- 90° or +/- 270°) they are shifted too far. In that case you should only shift by the height (not width) of the netlabel (which could actually be considered to always be 2.54mm).

@alfishe

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Since I was targeting only mirror operator - rotation should not be affected at all.
Could you please create separate bug for rotation? Would be happy to make it work as well.

@ubruhin

This comment has been minimized.

Copy link
Member

commented May 19, 2019

Sure, I was talking about the mirror operation. For example this:

grafik

gets mirrored to this:

grafik

So the vertical netlabels are shifted too far. This issue is introduced with this PR, it didn't exist before.

alfishe added some commits May 19, 2019

Selective mirroring only for horizontally rotated net labels.
Fixed snapping label to grid after mirror adjustments
@alfishe

This comment has been minimized.

Copy link
Contributor Author

commented May 19, 2019

Fixed for vertically oriented labels. Re-introduced snapping to grid after making adjustment.

@alfishe

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Combination of Mirror->Mirror->Rotate->Mirror->Rotate->Mirror in some combinations gives weird results with random offsets to left and right. But seems it has close correlation with overall elements shift. So all operations should have better rotation/mirror center than now.

@ubruhin

This comment has been minimized.

Copy link
Member

commented May 20, 2019

Thanks for the update!

Combination of Mirror->Mirror->Rotate->Mirror->Rotate->Mirror in some combinations gives weird results with random offsets to left and right. But seems it has close correlation with overall elements shift.

Maybe that's because you check only for 0° and 180°, but not for -180°. You can check it when first rotating the items clockwise (Shift+R) and mirror afterwards -> offset of netlabels.

Could you fix this too? Then I would say this is ready to merge. Btw, I think qFuzzyCompare is not needed - you could compare the Angle object directly with e.g. Angle::deg180() as this way no floating point numbers are involved (i.e. == works fine). Ah, and you could use Angle::mappedTo0_360deg() to avoid comparing with both +180° and -180°.

@alfishe

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

You are tough maintainer (probably in a good sense =)). But totally makes sense. Committed changes

@dbrgn

This comment has been minimized.

Copy link
Member

commented May 23, 2019

You are tough maintainer (probably in a good sense =)). But totally makes sense. Committed changes

In my experience @ubruhin almost always sees something to criticize, but then he's almost always right 😄

@ubruhin

This comment has been minimized.

Copy link
Member

commented May 24, 2019

Looks good now, thanks for your contribution @alfishe! And sorry for my tough review 😁

@ubruhin ubruhin added this to the 0.1.2 milestone May 24, 2019

@ubruhin ubruhin changed the title Fix for issue #335 - SchematicEditor: Net labels are not mirrored properly SchematicEditor: Fix net label offset after mirroring May 24, 2019

@ubruhin ubruhin merged commit 9be8db0 into LibrePCB:master May 24, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alfishe

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Thank you for guiding me into existing codebase capabilities =) Looking for more focused / feature development job here.

ubruhin added a commit that referenced this pull request Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.