Move to float32 coordinates #1661

andydotxyz · 2020-12-16T16:48:33Z

Move coordinates to float32.
Move text size as well.

Add new Vector type for mouse events (also float32s).
More interoperability between Size/Position/Vector.

Checklist:

Tests included.
Lint and formatter run with no errors.
Tests all pass.
Any breaking changes have a deprecation path or have been discussed.

andydotxyz · 2020-12-16T19:32:25Z

What's the purpose of IsZero()? We could just as well implement this as func IsZero(v Vec2) bool.

It was in there already for the Size and Position types, I did not want to remove it.
Overall I thought we were trying to avoid package exported functions like that? I can happily remove it from the interface, but given that all the types have it, it seemed logical to keep it there.

charlesdaniels · 2020-12-16T20:43:06Z

I have no problem it it, it just seemed like something that could be done as a function that takes the interface instance as an arg. It's pretty trivial either way.

toaster

A couple of questions and opinions. See comments and:

The alignment of colour channel sliders and submenu arrows looked better before, IMO. Should we adjust that?
Unfortunately I don’t have the time to read all discussions on Slack. A PR should explain its intent or the decision it is based on, but this doesn’t. Why should we make this move?
The removal of the deprecated Union would have been better in an own PR, IMO.
The same goes for the new Vector type.
At least one commit (058e998) contains stuff that belongs to earlier commits :/. That might look like nit picking but I think focused commits help a lot with the review process. That’s why I put a significant amount of time into diff review before creating a commit and into commit clean-up before creating a PR.

canvas/text_test.go

toaster · 2020-12-18T06:23:53Z

test/markup_renderer.go

-func (r *markupRenderer) setIntAttrWithDefault(attrs map[string]*string, name string, i int, d int) {
-	if i == d {
+func (r *markupRenderer) setIntAttrWithDefault(attrs map[string]*string, name string, i float32, d float32) {
+	if int(i) == int(d) {


I’d prefer to not use float32 as input to a method that’s intended to process int values.
AFAICS it’s only used for text size. Since text size now is float, we might use setFloatAttrWithDefault instead and remove setIntAttrWithDefault?

Thanks, I missed the name for some reason

test/markup_renderer.go

widget/button_internal_test.go

internal/driver/glfw/driver_test.go

toaster · 2020-12-18T06:59:24Z

geometry.go

@@ -1,5 +1,12 @@
 package fyne

+// Vec2 marks geometry types that can operate as a coordinate vector


What does Vec2 mean?
What is its purpose?
Is there a better name to it?
I think Get() is named to generic. If it returns the elements of the vector, why isn’t it called Elements()? Or Components()?

When thinking longer about this, it seems that even Vector is too generic. Maybe Coordinate is better? It might even be necessary to include the amount of dimensions in the name.

https://github.com/fyne-io/fyne/wiki/Proposal:-Use-Vectors-to-Represent-Position,-Size,-Etc.

I will see if the other name suggestions work better

I don't think Coordinate is better than Vector. It is generic on purpose - it can refer to a coordinate, a size, a delta etc. It's just X and Y quantity of some sort, and I can't find a better name for it

Of course we could make Vector a more specific XYDelta (as that is what what it's been used for so far), and then rename Vec2 as Vector or Vector2.
The rest of the naming is now tidied up.

Okay. I like the idea of using Vector for the interface.
The mathematical correct name for the current Vector implementation would be Displacement (https://en.wikipedia.org/wiki/Displacement_(geometry)). Unless you or someone else has a better idea, I would favour this one.

The mathematical correct name for the current Vector implementation would be Displacement

Good point, though not a commonly used phrase and has a very specific meaning whereas this is trying to be quite generic. Delta feels more commonly used, or we could even go simpler with Move - this will be alongside Size and Position so looking for something commonplace word.

I’m okay with Move or Movement but don’t really like Delta :).

andydotxyz · 2020-12-18T10:16:54Z

Answers to the questions above that did not relate to lines of code (answered inline):

The alignment of colour channel sliders and submenu arrows looked better before, IMO. Should we adjust that?

This change does not intentionally move anything, it is a result of the rounding/truncating being removed.
If we want to make specific adjustments after this that is OK, but I did not want to go ahead and do that as part of the coordinate type change.

Unfortunately I don’t have the time to read all discussions on Slack.

By using int in a HiDPI environment you cannot actually make use of each pixel position / size - for example on macOS Retina if we move an element it will jump 2 pixels. on iOS and Android this gets worse as SCALE is 3, 3.5 and 4 - so we see moves jump up to 4 pixels. This change allows us to do "sub-pixel" on the scalable layer leading to more fine-grained output.

The removal of the deprecated Union would have been better in an own PR, IMO.

Maybe, but this is a re-working of geometry, of which the Union is part, so it seemed related enough and avoided a separate, tiny, PR.

The same goes for the new Vector type.

As above, I guess you could argue that, but it's all related.

At least one commit (058e998) contains stuff that belongs to earlier commits :/. That might look like nit picking but I think focused commits help a lot with the review process. That’s why I put a significant amount of time into diff review before creating a commit and into commit clean-up before creating a PR.

Apologies for this. As you can imagine this change took 2 full days to perform and polish. I suspected that unpicking, cherry-picking and reworking the commits would have taken another half day which didn't seem like it offered value for time.

toaster · 2020-12-19T16:53:37Z

This change does not intentionally move anything, it is a result of the rounding/truncating being removed.
If we want to make specific adjustments after this that is OK, but I did not want to go ahead and do that as part of the coordinate type change.

Okay, I just filed #1674 for this.

* The removal of the deprecated Union would have been better in an own PR, IMO.
Maybe, but this is a re-working of geometry, of which the Union is part, so it seemed related enough and avoided a separate, tiny, PR.
* The same goes for the new Vector type.
As above, I guess you could argue that, but it's all related.

I understand your argument but nevertheless disagree. Both of these changes make complete sense on their own, without the int->float32 transformation.
I understand that such things often happen as by-product of some development. I experience this on my own nearly every day. Nevertheless, it is a good idea to separate these into own PRs (even if they are tiny). This helps to keep the original PR focused and thus comprehensible and reviewable. You don’t do such commit optimisation for yourself (well, maybe a little bit) but for the reviewers and at least I would greatly appreciate this.
Sorry, for being such a nuisance about this but I think it pays out in the end by making the review process as smooth as possible.

Apologies for this. As you can imagine this change took 2 full days to perform and polish. I suspected that unpicking, cherry-picking and reworking the commits would have taken another half day which didn't seem like it offered value for time.

Apology accepted, subterfuge not.
Big changes take time and often create a lot of stress. I know this and have to discipline myself over and over to not rush out things just to be done with them. Rest assured that such improvements to your commits and PRs are worth the effort by saving you and the reviewer time during the review process.
There are tools that help to make these operations less expensive. I’m using GitUp for this but I think there are even better alternatives. Given the three things I complained about, I would expect that re-arranging them into own PRs would not take longer than two hours. Yes, time is valuable but sometime investing a little bit in one place pays out in another one.

Sorry if some of this seems harsh, I don't want to offend you but I wanted to make my point clear.

toaster · 2020-12-19T16:55:26Z

P.S.: Despite my long rant, I don’t expect that you extract any of these things into a separate PR now. I just want to suggest this for future PRs.

toaster · 2020-12-19T17:03:36Z

geometry.go

 	IsZero() bool
 }

-// Vector is a generic X, Y coordinate type
+// Vector is a generic X, Y coordinate or size representation.


And movement (or displacement) :).

andydotxyz · 2020-12-19T17:03:47Z

P.S.: Despite my long rant, I don’t expect that you extract any of these things into a separate PR now. I just want to suggest this for future PRs.

Thanks for the clarification, I was worried about lots of required changes until I read this :)

stuartmscott

Looks good. I like that we got rid of a bunch of casts, tho it is a shame there isn't a way to define 32bit float point literals without the cast.

The color slider looks more inline with the label, so I think this change actually improves it.

stuartmscott · 2020-12-19T16:35:16Z

canvas/line.go

@@ -24,8 +24,8 @@ type Line struct {

 // Size returns the current size of bounding box for this line object
 func (l *Line) Size() fyne.Size {
-	return fyne.NewSize(int(math.Abs(float64(l.Position2.X)-float64(l.Position1.X))),
-		int(math.Abs(float64(l.Position2.Y)-float64(l.Position1.Y))))
+	return fyne.NewSize(float32(math.Abs(float64(l.Position2.X)-float64(l.Position1.X))),


This 'downcasting to float32 from the math library's float64' pattern could end up being quite common. Could we instead add fyne.Abs alongside fyne.Min & fyne.Max?

Yes I think we could add a few more helpers building on the new types

andydotxyz · 2020-12-19T17:17:18Z

Sorry if some of this seems harsh, I don't want to offend you but I wanted to make my point clear.

Not at all - points taken. Perhaps I am running too fast at some of this stuff.
Trying to coordinate A book launch and 2.0 at the same time was probably a bad idea.

toaster · 2020-12-19T18:15:54Z

Because the discussion grow to some length, here’s a checklist of the remaining tasks from my wishlist:

don’t send floats to a method for int; either,
- drop MarkupRenderer’s setIntAttrWithDefault and use setFloatAttrWithDefault instead, or
- cast before calling setIntAttrWithDefault and add the move to setFloatAttrWithDefault task to Improve precision of markup test XML #1672, or
- rename setIntAttrWithDefault to setScalarSizeAttrWithDefault (and still add a note to Improve precision of markup test XML #1672)
improve naming of vector types and interface

Also ScrollEvent.Delta to ScrollEvent.Scrolled (as we are breaking anyway).

andydotxyz · 2020-12-19T18:31:10Z

I think the Int naminng was fixed yesteday @toaster - I moved it to Size based name, so it's less generic.

The other naming is now updated too.
In the chat channel Delta with DX and DY made a lot of sense. Let me know what you think :)

Move geometry from int to float32.

0fde789

Fixes fyne-io#835

andydotxyz changed the base branch from master to develop December 16, 2020 16:50

andydotxyz added 6 commits December 16, 2020 18:14

Fix missed tests

a233a15

remove deprecated Size.Union

c594f47

Move mouse events to Vector from (float32, float32)

058e998

Add Vec2 for interoperability between Size, Position and Vector

c0644a5

Fix missed format somehow

83cf0d8

Fix broken format fix

32606da

andydotxyz marked this pull request as ready for review December 16, 2020 19:34

andydotxyz changed the title ~~Fix/835~~ Move to float32 coordinates Dec 16, 2020

toaster requested changes Dec 18, 2020

View reviewed changes

Fix naming from PR

697fcf5

andydotxyz requested a review from toaster December 18, 2020 22:32

toaster mentioned this pull request Dec 19, 2020

Improve alignment of colour sliders and submenu arrows #1674

Open

toaster reviewed Dec 19, 2020

View reviewed changes

stuartmscott reviewed Dec 19, 2020

View reviewed changes

Updated naming further - Vec2 -> Vector2, Vector -> Delta

35c075c

Also ScrollEvent.Delta to ScrollEvent.Scrolled (as we are breaking anyway).

andydotxyz requested a review from toaster December 19, 2020 18:31

toaster approved these changes Dec 19, 2020

View reviewed changes

andydotxyz merged commit e30a503 into fyne-io:develop Dec 19, 2020

andydotxyz deleted the fix/835 branch December 19, 2020 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to float32 coordinates #1661

Move to float32 coordinates #1661

andydotxyz commented Dec 16, 2020 •

edited

Loading

andydotxyz commented Dec 16, 2020

charlesdaniels commented Dec 16, 2020

toaster left a comment

toaster Dec 18, 2020

andydotxyz Dec 18, 2020

toaster Dec 18, 2020

andydotxyz Dec 18, 2020

andydotxyz Dec 18, 2020

andydotxyz Dec 18, 2020

toaster Dec 19, 2020

andydotxyz Dec 19, 2020

toaster Dec 19, 2020

andydotxyz commented Dec 18, 2020 •

edited

Loading

toaster commented Dec 19, 2020

toaster commented Dec 19, 2020

toaster Dec 19, 2020 •

edited

Loading

andydotxyz commented Dec 19, 2020

stuartmscott left a comment

stuartmscott Dec 19, 2020

andydotxyz Dec 19, 2020

andydotxyz commented Dec 19, 2020

toaster commented Dec 19, 2020 •

edited

Loading

andydotxyz commented Dec 19, 2020

		@@ -1,5 +1,12 @@
		package fyne

		// Vec2 marks geometry types that can operate as a coordinate vector

Move to float32 coordinates #1661

Move to float32 coordinates #1661

Conversation

andydotxyz commented Dec 16, 2020 • edited Loading

Checklist:

andydotxyz commented Dec 16, 2020

charlesdaniels commented Dec 16, 2020

toaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Dec 18, 2020 • edited Loading

toaster commented Dec 19, 2020

toaster commented Dec 19, 2020

toaster Dec 19, 2020 • edited Loading

Choose a reason for hiding this comment

andydotxyz commented Dec 19, 2020

stuartmscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz commented Dec 19, 2020

toaster commented Dec 19, 2020 • edited Loading

andydotxyz commented Dec 19, 2020

andydotxyz commented Dec 16, 2020 •

edited

Loading

andydotxyz commented Dec 18, 2020 •

edited

Loading

toaster Dec 19, 2020 •

edited

Loading

toaster commented Dec 19, 2020 •

edited

Loading