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

PlotWindow: add an option to get tape measure #3959

Merged
merged 7 commits into from
Dec 11, 2023
Merged

PlotWindow: add an option to get tape measure #3959

merged 7 commits into from
Dec 11, 2023

Conversation

payno
Copy link
Member

@payno payno commented Oct 30, 2023

Changelog:

add tape measure action

screenshot

Screenshot from 2023-11-16 12-56-40

@payno payno changed the title PlotWindow: add a prototype for tape measure DRAFT: PlotWindow: add a prototype for tape measure Oct 30, 2023
@payno payno force-pushed the fix_3538 branch 2 times, most recently from 9140626 to 7b8976a Compare October 30, 2023 10:10
@vallsv
Copy link
Contributor

vallsv commented Oct 31, 2023

Sounds good to me.

And to answer to your message about displaying or not start/end points, i think it is not needed, and it's counter intuitive to display them. If you want to mesure something, just display the measure. If you need coords, you already have the tooltip or the status bar.

For other stuffs like coef or ndigits, this can be handled by overwrite of method.
You just have to provide the default behaviour.

class Ruler:
    ...

    # One can easely override that to change the digits, the scale, the unit
    def getLabelFromDeltaPixels(self, dx: float, dy: float):
        return f"{sqrt(x*x + y*y):0.2f} px"

If you provide such component, that's something i will integrate directly in flint.

@vallsv
Copy link
Contributor

vallsv commented Oct 31, 2023

About the code

  • I would not add it by default in the plots, that's a tool you can integrate yourself
  • I would split what is about the toolbutton and what is about the ruler behaviour itself (but i am fine with it for now, it's easier to override with a single class)
  • I would create an even minimalistic icon, else it will not be readable (i.e. https://www.creativefabrica.com/fr/product/ruler-icon-2/ with even less details). Tell me if you want i can do the stuff.

@payno
Copy link
Member Author

payno commented Nov 2, 2023

thanks, don't worry I can do it all.

* rename TapeMeasureToolButton to RulerToolButton
* update icon
* allow user to provide the color to be used for the ruler
* add implementation of '_disconnectPlot' to improve robustness
* define 'format_distance' function to ease redefinition of the format to apply to the distance
* remove default implemtation by the PlotWindow
* add an example (overkill ?)
@payno payno changed the title DRAFT: PlotWindow: add a prototype for tape measure PlotWindow: add a prototype for tape measure Nov 16, 2023
@payno payno requested a review from vallsv November 22, 2023 12:47
@payno payno changed the title PlotWindow: add a prototype for tape measure PlotWindow: add an option to get tape measure Dec 8, 2023
@payno
Copy link
Member Author

payno commented Dec 8, 2023

@vallsv are you fine with the PR ? Does it fit your needs ?

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

It would be nice to add the sample code and the RulerToolButton to the documentation

@t20100 t20100 added this to the 2.0.0 milestone Dec 8, 2023
@vallsv
Copy link
Contributor

vallsv commented Dec 8, 2023

I haven't cheak the interaction, i have not much time but if think:

  • format_distance -> formatDistance
  • format_distance must not be a class method, else it's pointless i can't override it
  • you should cut the ruler icon by 3, there is no need to have realistic image -> only 2 or 3 tick is enough -> as result you can have a larger ruler for readability

I also would like to tune the line style, but i could try to do it later

I also think it's not the best idea to use a ROI here, because you need a dedicated roi manager for a single roi, but if it does the job, it's fine.

@vallsv
Copy link
Contributor

vallsv commented Dec 8, 2023

I think format_distance should have dx and dy as argument, you can't expect the coefficient is the same for x and y. But well, this can be considered as another use case.

@payno
Copy link
Member Author

payno commented Dec 8, 2023

Having 'format_distance' not being a 'staticmethod' is not that straightforward sadly.

using RulerROI with the roi manager is 'breaking' the link between the RulerToolButton and the RulerROI.
So we can change the roi manager to allow providing other parameters or avoid using manager (as you suggested on a previous command). But in this case we need to redo all the management of interaction.

An alternative to avoid having heavy modifications is the commit '917ff73fb'. This is not very elegant but it will do the job and should allow us to extend the class as we wish. Else I guess we can also do something with metaclasses but again it will be against readability.

@vallsv
Copy link
Contributor

vallsv commented Dec 8, 2023

Thanks for the fix.
Looks good to me.

@payno payno merged commit 71acbac into main Dec 11, 2023
8 checks passed
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.

3 participants