[PATCH] GUI: Implement a true rotary potmeter
The 'potmeter' currently responds only to horizontal mouse coordinates. This is especially confusing when the pot does not rotate in the expected direction depending on the quadrants of pot and mouse positions. Attached patch implements a true rotary pot. You can click on an angular position and rotate by circular mouse movement. And it has stop points like its mechanical counterpart. Tested with the volume pot of default skin 'Blue'. The patch is quite big due to the code redundancy in the files. Perhaps outsourcing and reusing common code? Hans-Dieter
Hans-Dieter Kosch wrote on Sun, 16 Feb 2014 23:09:01 +0100:
Attached patch implements a true rotary pot.
Please apply rpotmeter.patch, implement the "/* !!! */" (add code or remove it if not necessary) and post this diff of yours then. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Sun, 16 Feb 2014 23:09:01 +0100:
Attached patch implements a true rotary pot.
Please apply rpotmeter.patch, implement the "/* !!! */" (add code or remove it if not necessary) and post this diff of yours then.
Great! However, I'm somewhat out of time at the moment, hope to manage it next weekend. At this point, I came across 2 issues: 1) If numphases == 1, hpotmeter gets its phase image drawn partially according to the percentage of the value to be displayed (SinglePhasePutImage), while vpotmeter gets it always fully drawn. How to cope with this inconsistency? 2) If numphases == 0, we get a division by zero in the rendering (PutImage). Should this be trapped? Just an idea: Treat numphases = 0 like numphases = 1, but handle it as special case to give the hpotmeter an always full phase image; admittedly, a bad hack for backward compatibility. Hans-Dieter
Hans-Dieter Kosch wrote on Tue, 18 Mar 2014 02:03:28 +0100:
At this point, I came across 2 issues:
And my to-do list grows and grows... Although render.c has already been converted into MPlayer coding style, the code itself hasn't yet been fully reviewed. Seems to be time to do so now. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Tue, 18 Mar 2014 02:03:28 +0100:
At this point, I came across 2 issues:
And my to-do list grows and grows...
I'd volunteer to help...
Although render.c has already been converted into MPlayer coding style, the code itself hasn't yet been fully reviewed. Seems to be time to do so now.
Right, readable coding style is valuable help for review. On and off, I notice something in the code, maybe I should post a note or a patch from time to time. Hans-Dieter
Hans-Dieter Kosch wrote on Tue, 18 Mar 2014 02:03:28 +0100:
At this point, I came across 2 issues:
1) If numphases == 1, hpotmeter gets its phase image drawn partially according to the percentage of the value to be displayed (SinglePhasePutImage), while vpotmeter gets it always fully drawn. How to cope with this inconsistency?
Nothing in the documentation indicates that numphases == 1 will be treated specially (and I don't see any point in not getting all phases by the skin designer), so I'd say: bye-bye, SinglePhasePutImage(). (With a SinglePhasePutImage(), we'd have two, actually three or four different kinds of phase images, a multi-phase one and a special one-phase ones for each potmeter - fracturing width, height and(?) angle(?)... No dice! Fortunately, no skin is using a one-phase image.)
2) If numphases == 0, we get a division by zero in the rendering (PutImage). Should this be trapped?
No, I'd say numphases == 0 is an erroneous and invalid skin entry which ought result in a skin reject. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Tue, 18 Mar 2014 02:03:28 +0100:
1) If numphases == 1, hpotmeter gets its phase image drawn partially according to the percentage of the value to be displayed (SinglePhasePutImage), while vpotmeter gets it always fully drawn. How to cope with this inconsistency?
Nothing in the documentation indicates that numphases == 1 will be treated specially (and I don't see any point in not getting all phases by the skin designer), so I'd say: bye-bye, SinglePhasePutImage().
Yes, really, get rid of it.
2) If numphases == 0, we get a division by zero in the rendering (PutImage). Should this be trapped?
No, I'd say numphases == 0 is an erroneous and invalid skin entry which ought result in a skin reject.
'reject' is what I actually meant by 'trapped'. Meanwhile I think, simply let it run into SIGFPE as is. There can be more done wrong in skin definition which would be huge effort to 'trap'. A skin designer should really know what he/she's doing. Hans-Dieter
Hans-Dieter Kosch wrote on Fri, 28 Mar 2014 03:02:18 +0100:
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Tue, 18 Mar 2014 02:03:28 +0100:
2) If numphases == 0, we get a division by zero in the rendering (PutImage). Should this be trapped?
No, I'd say numphases == 0 is an erroneous and invalid skin entry which ought result in a skin reject.
'reject' is what I actually meant by 'trapped'. Meanwhile I think, simply let it run into SIGFPE as is. There can be more done wrong in skin definition which would be huge effort to 'trap'. A skin designer should really know what he/she's doing.
By 'reject' I meant: display an error and exit. A skin designer can do a lot of weird things, but he/she mustn't be able to cause a crash. Ingo
Hi Ingo, Ingo Brückl wrote:
Please apply rpotmeter.patch, implement the "/* !!! */" (add code or remove it if not necessary) and post this diff of yours then.
For good coding style, I need a symbolic constant #define RPOT_STOP_GAP (1/60.0) to be used in gui/ui/main.c gui/ui/playbar.c gui/ui/render.c gui/win32/gui.c gui/win32/widgetrender.c I'd like to put this into a common header file, but where... Do you have a suggestion? Hans-Dieter
Hans-Dieter Kosch wrote on Sat, 22 Mar 2014 04:11:46 +0100:
For good coding style, I need a symbolic constant
#define RPOT_STOP_GAP (1/60.0)
Please be verbose with names (RPOTMETER_STOP_GAP).
I'd like to put this into a common header file, but where... Do you have a suggestion?
app/gui.h Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Sun, 16 Feb 2014 23:09:01 +0100:
Attached patch implements a true rotary pot.
Please apply rpotmeter.patch, implement the "/* !!! */" (add code or remove it if not necessary) and post this diff of yours then.
Here now the patch for the GTK GUI, WIN GUI will follow soon (not yet really sure about). Diff'd against current SVN. I've placed comments in the code for better understanding. At your discretion, keep, remove, or rewrite them. Hans-Dieter
Hans-Dieter Kosch wrote on Mon, 31 Mar 2014 01:44:10 +0200:
Here now the patch for the GTK GUI
First of all, nice work! Looking through your patch was a nice brush-up in trigonometry. ;-) I have a few first questions and comments though.
+ double dx = X - item->x - item->width / 2; + double dy = Y - item->y - item->height / 2; + + last_rpotmeter_angle = (dx == 0 && dy == 0) ? 0 : (atan2(dx, -dy) + M_PI) / (2*M_PI);
The variable name (last_rpotmeter_angle) is confusing, since stricly speaking it isn't an angle (in degrees). I was expecting something like "atan2(-dy, dx) / M_PI * 180" here. Well, it seems that you've fixed where 0% is. Have you given plastic skin a go? ;-) It is hard to say where 0 for a poti should be (I actually don't remember the old radios with potis that snapped and clicked into place), but since the zero point is arbitrary I'd say we need a different approach. (And think of these modern "endless" rotary potis you have nowadays, a 1-phase potmeter or a potmeter without mark/button. Where is its 0%?) I've not completely thought about it, but it seems that we would need some additional information about the zero point of a rpotmeter, or mouse clicks can't be handled else. A mouse drag simply is a difference to the degree/radian from wsPLMouseButton and the item's value should be adjusted according to this difference then. For the mouse wheel it's just a simple value adjustment (currently unchecked by the way), but a mouse click is tricky. Since rpotmeters don't yet exist at the moment, we can set up the rules for them without breaking existing skins. However, I want to remain as compatible as possible with the other potmeters. What do you think?
+ // CW angle, 0..1, 0 at center/bottom
You should be verbose (CW in this case; clockwise?).
+ a = FFMAX( a, RPOTMETER_STOP_GAP ); + a = FFMIN( a, 1-RPOTMETER_STOP_GAP ); + value = ((a - RPOTMETER_STOP_GAP) / (1-2*RPOTMETER_STOP_GAP)) * 100.0;
How did you determine RPOTMETER_STOP_GAP?
+ // Stop rotating when crossing 0 until crossing in opposite direction + if (last_rpotmeter_angle < 0.25 && a > 0.75) rpotmeter_stop = rpotmeter_stop == 1 ? 0 : -1; + if (last_rpotmeter_angle > 0.75 && a < 0.25) rpotmeter_stop = rpotmeter_stop == -1 ? 0 : 1;
Where do the numeric constants (0.25, 0.75) come from? Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Mon, 31 Mar 2014 01:44:10 +0200:
I have a few first questions and comments though.
+ double dx = X - item->x - item->width / 2; + double dy = Y - item->y - item->height / 2; + + last_rpotmeter_angle = (dx == 0 && dy == 0) ? 0 : (atan2(dx, -dy) + M_PI) / (2*M_PI);
The variable name (last_rpotmeter_angle) is confusing, since stricly speaking it isn't an angle (in degrees).
Oh, yes, convenient naming is sometimes half the coding effort. It's a normalised angle 0..1 as stated later in a comment. I do not object against finding a better name.
I was expecting something like "atan2(-dy, dx) / M_PI * 180" here.
All trigonometric functions work with radians, not degrees; so 180 would be M_PI. That expression calculates in one step the angle mirrored from mathematical counter clockwise to physical clockwise, rotates the 0 from middle right to center bottom and normalises it to 0..1. In the beginning, I had it in separate steps for better understanding, but finally, I didn't dare posting such exploded code. The other [hv]potmeter calculations are also very compactly coded.
Well, it seems that you've fixed where 0% is. Have you given plastic skin a go? ;-)
Not yet, I'll look.
It is hard to say where 0 for a poti should be (I actually don't remember the old radios with potis that snapped and clicked into place), but since the zero point is arbitrary I'd say we need a different approach. (And think of these modern "endless" rotary potis you have nowadays, a 1-phase potmeter or a potmeter without mark/button. Where is its 0%?)
Right, the zero point is arbitrary, I thought about this, too. Traditional mechanical potis have it at center bottom, but I've also seen other species. Specifying the zero point would need a skin definition and appears somewhat luxury to me. The endless potis are designed for incremental adjusting of different values depending on an item selection. The item value, of course, has limits and needs a separate display. And it has to be obvious which item is about to be adjusted. All this comes from modern complicated narrowly layouted multi function interfaces. Do we want such? In the skin definition, the function of a poti is clearly assigned.
I've not completely thought about it, but it seems that we would need some additional information about the zero point of a rpotmeter, or mouse clicks can't be handled else.
Mouse clicks work like with the other potis, position and value jump to the clicked point (GTK GUI).
A mouse drag simply is a difference to the degree/radian from wsPLMouseButton and the item's value should be adjusted according to this difference then.
Dragging purely by difference is WIN GUI behaviour (untested as said). The GTK GUI sets the point according to the mouse button down position and proceeds from there on; this works like with the other potis.
For the mouse wheel it's just a simple value adjustment (currently unchecked by the way), but a mouse click is tricky.
The wheel is tested to work flawlessly (with all potis).
Since rpotmeters don't yet exist at the moment, we can set up the rules for them without breaking existing skins. However, I want to remain as compatible as possible with the other potmeters.
What do you think?
OK. Compatibility first, but open for any ideas. I regard a rotary poti just like a linear one. The difference is that a circle has basically no start and no end...
+ // CW angle, 0..1, 0 at center/bottom
You should be verbose (CW in this case; clockwise?).
Yes, CW (clockwise) and CCW (counter clockwise) are usual abbreviations. I was anyway unsure about commenting. I'm used to comprehensively comment my code for easy understanding, but found it rarely in MPlayer code.
+ a = FFMAX( a, RPOTMETER_STOP_GAP ); + a = FFMIN( a, 1-RPOTMETER_STOP_GAP ); + value = ((a - RPOTMETER_STOP_GAP) / (1-2*RPOTMETER_STOP_GAP)) * 100.0;
How did you determine RPOTMETER_STOP_GAP?
This is more or less arbitrarily chosen. Its value 1/60 corresponds to 1 minute of angle, and matches quite good the 51 image phases of the volume potmeter in the 'default' test skin. Traditional mechanical potis have a 270° range, so RPOTMETER_STOP_GAP could be 1/8, but this appeared too restrictive to me.
+ // Stop rotating when crossing 0 until crossing in opposite direction + if (last_rpotmeter_angle < 0.25 && a > 0.75) rpotmeter_stop = rpotmeter_stop == 1 ? 0 : -1; + if (last_rpotmeter_angle > 0.75 && a < 0.25) rpotmeter_stop = rpotmeter_stop == -1 ? 0 : 1;
Where do the numeric constants (0.25, 0.75) come from?
They separate between the upper and lower half of the potmeter circle. The stop gap is in the lower half: 0.5 0.25 0.75 0.0|1.0 Hans-Dieter
Hans-Dieter Kosch wrote on Wed, 02 Apr 2014 04:24:47 +0200:
Ingo Brückl wrote:
Well, it seems that you've fixed where 0% is. Have you given plastic skin a go? ;-)
Not yet, I'll look.
Please do so, and I'm sure there are a few interesting other ones.
It is hard to say where 0 for a poti should be
Right, the zero point is arbitrary, I thought about this, too.
But with your patch it's fixed.
Specifying the zero point would need a skin definition and appears somewhat luxury to me.
Well, to me it seems absolutely necessary. And after having thought about it for a while (which I should have done earlier), we not only need to know about the 0% point but the 100% point, too.
The endless potis are designed for incremental adjusting of different values depending on an item selection. The item value, of course, has limits [...] Do we want such?
Why not, if the implementation is universal enough to handle this without extra effort.
I've not completely thought about it, but it seems that we would need some additional information about the zero point of a rpotmeter, or mouse clicks can't be handled else.
Mouse clicks work like with the other potis, position and value jump to the clicked point (GTK GUI).
Yes, but I'm speaking of a potmeter which unknown zero point. You only know the one from the standard skin, because you've looked into the image file and interpreted it. ;-) While mouse dragging and wheeling is possible without knowing the zero point, clicking isn't (at least, it seems this way to me).
Since rpotmeters don't yet exist at the moment, we can set up the rules for them without breaking existing skins. However, I want to remain as compatible as possible with the other potmeters.
What do you think?
OK. Compatibility first, but open for any ideas. I regard a rotary poti just like a linear one.
Agreed, but for linear ones you always know the 0% and 100% points.
The difference is that a circle has basically no start and no end...
Unless you get this information... (not about the circle, but the potmeter). I've thought about a more general approach for rpotmeters and I think that it is necessary to get the end point information (0% and 100%) from the designer. The problem is that I want to remain compatible and I don't want to overexert the designer. He/she shouldn't have to think about degrees or some trigonometric functions, but as is usual in pixels. Well, this is what I've come up with: For a rpotmeter the default value of the definition has to be default;x;y;X;Y where x,y is the coordinate (on the edge of the potentiometer image) of the 0% point in phase#1 and X,Y the one for the 100% in phase#n - in other words the tip of the mark on the potentiometer image. These (their atan2 probably) will be stored in guiItem which makes it possible to consider them during calculation, and the gap is calculable now. This should allow all kinds of rotary potentiometers. What do you think? Do you feel like proceeding? Ingo PS: For clarity, I'd prefer the (argument-wise) more straightforward transformation atan2(-dy, -dx) resulting in 0.75 0.5 0.0/1.0 0.25 unless this turns out to be impractical.
Just a quick response for now, I'll do more on weekend. Ingo Brückl wrote:
These (their atan2 probably) will be stored in guiItem which makes it possible to consider them during calculation, and the gap is calculable now.
If so, fine. I tried to avoid hacking guiItem.
What do you think? Do you feel like proceeding?
Not bad. Of course, I'll proceed. Perhaps, we should work out a specification defining the exact behaviour of all potmeters (both GTK and WIN); how they react on mouse events, how the item value is derived thereof, how the visible representation results from the item value; so that all matches, also a value set by the mouse or a value coming from other sources (e.g. video position). Then, we'd have an agreed base to review and rework coding. Hans-Dieter
Hans-Dieter Kosch wrote on Wed, 02 Apr 2014 04:24:47 +0200:
I regard a rotary poti just like a linear one. The difference is that a circle has basically no start and no end...
Well, what about the following then? Let the default definition for a rpotmeter be default;x;y;X;Y. Now be: start = atan2 transformation of x,y end = atan2 transformation of X,Y range = end - start (add 1 if <= 0) gap = 1 - range To handle the rpotmeter similar to the linear ones, let a point x,y clicked on be p: p = atan2 transformation of x,y - start (add 1 if <= 0) We have a kind of linear potmeter starting with 0 of width range now and calculating the item's value is simple: if p <= range then value = (p / range) * 100 else if p < range + gap / 2 then value = 100 else value = 0 So, guiItem only needs to provide start and range. (All names are just examples.) Attached is a quick and dirty proof of concept (for plastic skin and main.c only; although more universal, because it should work for arbitrary start and stop points, it seems simpler). Can you detect any faulty reasoning? Anything I might have missed? Ingo
Ingo Brückl wrote:
Attached is a quick and dirty proof of concept (for plastic skin and main.c only; although more universal, because it should work for arbitrary start and stop points, it seems simpler).
Can you detect any faulty reasoning? Anything I might have missed?
At first glance, the basic idea seems OK. (BTW. this technique matches my proposal to keep the button of linear poti centered under the mouse.) Hans-Dieter
Hans-Dieter Kosch wrote on Thu, 03 Apr 2014 02:59:20 +0200:
Ingo Brückl wrote:
Can you detect any faulty reasoning? Anything I might have missed?
At first glance, the basic idea seems OK. (BTW. this technique matches my proposal to keep the button of linear poti centered under the mouse.)
It wouldn't have been possible without your work and our discussion (and I admit that we should have defined the behaviour of a rotary potmeter prior to developing a patch). So please don't feel like your effort wasn't appreciated. It is indeed very much, and it's the base for the (more general) approach. I'm going to make some source changes now in order to prepare it. Ingo
I wrote on Thu, 03 Apr 2014 08:59:47 +0200:
I'm going to make some source changes now in order to prepare it.
Here is an elaborated version of the patch working in radian now which saves some operations and makes it easier to understand (e.g. when looking up atan2 and radian in the wiki). You can already test with the plastic skin when setting the required data using static void g (guiItem *item) { double end; item->zeropoint = appRadian(item, 48, 52); end = appRadian(item, 60, 25); item->arclength = end - item->zeropoint; if (item->arclength <= 0.0) item->arclength += 2 * M_PI; } and g(item) right after case itRPotmeter in main.c. Comments? Ingo
Ingo Brückl wrote:
I wrote on Thu, 03 Apr 2014 08:59:47 +0200:
I'm going to make some source changes now in order to prepare it.
Here is an elaborated version of the patch working in radian now which saves some operations and makes it easier to understand (e.g. when looking up atan2 and radian in the wiki).
Comments?
+ if ( point <= item->arclength ) value=100.0 * point / item->arclength; + else if ( point < item->arclength / 2.0 + M_PI ) value=100.0f; + else value=0.0f;
Now, since we have 'constraint()', wouldn't be this sufficient: value=100.0 * point / item->arclength; (As said, more on weekend...) Hans-Dieter
Hans-Dieter Kosch wrote on Fri, 04 Apr 2014 02:05:20 +0200:
Ingo Brückl wrote:
Comments?
+ if ( point <= item->arclength ) value=100.0 * point / item->arclength; + else if ( point < item->arclength / 2.0 + M_PI ) value=100.0f; + else value=0.0f;
Now, since we have 'constraint()', wouldn't be this sufficient:
value=100.0 * point / item->arclength;
I don't think so. The elses handle the gap and a click near the 0% inside the gap must return 0. Ingo
Hans-Dieter Kosch wrote on Fri, 04 Apr 2014 02:05:20 +0200:
(As said, more on weekend...)
I've finished the (hopefully) completely elaborated more universal version which is very much based on your patch (though comments and variable names may vary). It supports arbitrary start and stop points which must be defined in the skin configuration file: (plastic) rpotmeter = NULL, 0, 0, volume, 51, 0'49'52'60'26, 189, 126, 61, 57, evSetVolume (standard) rpotmeter = NULL, 0, 0, volume, 51, 0'10'36'28'37, 311, 17, 40, 40, evSetVolume I'd very much appreciate you looking over, commenting and checking it. Ingo
Ingo Brückl wrote:
I've finished the (hopefully) completely elaborated more universal version which is very much based on your patch (though comments and variable names may vary). It supports arbitrary start and stop points which must be defined in the skin configuration file:
(plastic) rpotmeter = NULL, 0, 0, volume, 51, 0'49'52'60'26, 189, 126, 61, 57, evSetVolume (standard) rpotmeter = NULL, 0, 0, volume, 51, 0'10'36'28'37, 311, 17, 40, 40, evSetVolume
I'd very much appreciate you looking over, commenting and checking it
Here my first findings: Your comments in the code are great, they really help understanding! Wish that MPlayer (and other code) had more of such! :-) gui/app/gui.h: I wouldn't bother this header with math, defining M_2PI. Just use '(2*M_PI)' in place, the compiler folds it into a single constant anyway. gui/skin/skin.c: The syntax using "'" appears somewhat strange to me. I see that you have chosen it for consistent parsing. Another solution could be to count the number of "," ahead. As the parsing checks for rpotmeter at that point anyway, we could check if there are 4 more ",". The special case 'x0 == x1 && y0 == y1' to determine if 'zeropoint' and 'stop' are the same seems not needed. There are more cases where this happens: x0 == 0 and x1 == 0 and y0 and y1 are in the same quadrant or vice versa. So, simply calculate 'stop' and apply it. gui/ui/main.c and gui/ui/playbar.c: 'wsRLMouseButton' must regard the stop gap like 'wsMoveMouse' does, otherwise the potmeter's position may jump away after dragging and releasing (see my original patch). That's a code redundancy I mentioned earlier. Your new file 'gui/util/misc.c' could eventually be a place to outsource and in turn include such common code. The stop gap doesn't work reliably for me. Seems to be dependent on speed of mouse movement, i.e. dependent on the actually captured positions. I'll come back with further details. Hans-Dieter
Hans-Dieter Kosch wrote on Mon, 07 Apr 2014 03:27:22 +0200:
gui/app/gui.h: I wouldn't bother this header with math, defining M_2PI. Just use '(2*M_PI)' in place, the compiler folds it into a single constant anyway.
Thanks, changed.
gui/skin/skin.c: The syntax using "'" appears somewhat strange to me.
Yeah, I don't like it much either, but...
I see that you have chosen it for consistent parsing. Another solution could be to count the number of "," ahead. As the parsing checks for rpotmeter at that point anyway, we could check if there are 4 more ",".
...I didn't want the rpotmeter to have more parameters than the other ones. (Well, it actually has to have and has more parameters, but I wanted to somehow hide it.) My first idea was to use the semicolon as delimeter, but it's already used for comments. So, do you have any idea how to nicely slip the four extra parameters?
The special case 'x0 == x1 && y0 == y1' to determine if 'zeropoint' and 'stop' are the same seems not needed. There are more cases where this happens: x0 == 0 and x1 == 0 and y0 and y1 are in the same quadrant or vice versa.
Oops, you're right.
So, simply calculate 'stop' and apply it.
Well, I did so at first, but I got a difference of something like 9E-17 after calculation for two same points. (Must have been late in the night when I simply chose the current solution, but I should have investigated.)
gui/ui/main.c and gui/ui/playbar.c: 'wsRLMouseButton' must regard the stop gap like 'wsMoveMouse' does, otherwise the potmeter's position may jump away after dragging and releasing (see my original patch).
You're right again.
That's a code redundancy I mentioned earlier. Your new file 'gui/util/misc.c' could eventually be a place to outsource and in turn include such common code.
I'm planning to add ui/common.c or ui/ui.c for such ui specific code later.
The stop gap doesn't work reliably for me. Seems to be dependent on speed of mouse movement, i.e. dependent on the actually captured positions.
At which speed do you encounter problems? Large gaps like the one in the plastic skin may be more reliable than small ones like the one in the standard skin. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Mon, 07 Apr 2014 03:27:22 +0200:
gui/skin/skin.c: The syntax using "'" appears somewhat strange to me.
Yeah, I don't like it much either, but...
...I didn't want the rpotmeter to have more parameters than the other ones. (Well, it actually has to have and has more parameters, but I wanted to somehow hide it.)
It can't be really hidden, as the rpotmeter in fact has more parameters. It's simply not the same as a linear poti. So, I see no problem giving it a skin definition differing from the linear ones (but of course as consistent as possible).
So, do you have any idea how to nicely slip the four extra parameters?
I'll think about and provide a suggestion.
The special case 'x0 == x1 && y0 == y1' to determine if 'zeropoint' and 'stop' are the same seems not needed. There are more cases where this happens: x0 == 0 and x1 == 0 and y0 and y1 are in the same quadrant or vice versa.
So, simply calculate 'stop' and apply it.
Well, I did so at first, but I got a difference of something like 9E-17 after calculation for two same points. (Must have been late in the night when I simply chose the current solution, but I should have investigated.)
I see, floating point precision issue. So, leave that trap and add traps for the cases where the points lie on the x/y axis. On the other hand, has such an inaccuracy a practical impact?
The stop gap doesn't work reliably for me. Seems to be dependent on speed of mouse movement, i.e. dependent on the actually captured positions.
At which speed do you encounter problems?
I can't reproduce it anymore. My observation was, that the poti didn't always stop at 100% when dragging the mouse. For now, all is working perfectly. Hans-Dieter
Hans-Dieter Kosch wrote on Tue, 08 Apr 2014 01:56:39 +0200:
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Mon, 07 Apr 2014 03:27:22 +0200:
The special case 'x0 == x1 && y0 == y1' to determine if 'zeropoint' and 'stop' are the same seems not needed. There are more cases where this happens: x0 == 0 and x1 == 0 and y0 and y1 are in the same quadrant or vice versa. So, simply calculate 'stop' and apply it.
Well, I did so at first, but I got a difference of something like 9E-17 after calculation for two same points. (Must have been late in the night when I simply chose the current solution, but I should have investigated.)
I see, floating point precision issue.
Yes, but why is it if x0,y0 == x1,y1? item->zeropoint = appRadian(item, x0, y0); item->arclength = appRadian(item, x1, y1) - item->zeropoint; results in item->arclength not being zero (for the same call with same arguments)? I could understand for *different* points lying on the same axis, but for identical points?
So, leave that trap and add traps for the cases where the points lie on the x/y axis. On the other hand, has such an inaccuracy a practical impact?
Yes, we have to know if there is a potmeter without gap, because item->arclength should be 2*pi then and not 9E-17 or such. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Tue, 08 Apr 2014 01:56:39 +0200:
I see, floating point precision issue.
Yes, but why is it if x0,y0 == x1,y1?
item->zeropoint = appRadian(item, x0, y0); item->arclength = appRadian(item, x1, y1) - item->zeropoint;
results in item->arclength not being zero (for the same call with same arguments)? I could understand for *different* points lying on the same axis, but for identical points?
Mhm, that's in deed strange. Probably still a precision issue when building the difference, eventually system dependent, if floating point calculations are done in hardware or software? Hans-Dieter
Hans-Dieter Kosch wrote on Mon, 07 Apr 2014 03:27:22 +0200:
'wsRLMouseButton' must regard the stop gap like 'wsMoveMouse' does, otherwise the potmeter's position may jump away after dragging and releasing (see my original patch).
Please apply this patch additionally for testing (playbar will be accordingly). Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Mon, 07 Apr 2014 03:27:22 +0200:
'wsRLMouseButton' must regard the stop gap like 'wsMoveMouse' does, otherwise the potmeter's position may jump away after dragging and releasing (see my original patch).
Please apply this patch additionally for testing (playbar will be accordingly).
It works :-)
--- gui/ui/main.c 2014-04-07 19:47:42.000000000 +0200 +++ gui/ui/main.c.new 2014-04-07 19:50:56.000000000 +0200 @@ -141,7 +141,10 @@ if ( itemtype == itRPotmeter ) { last_point=appRadian( item, X - item->x, Y - item->y ) - item->zeropoint; - endstop=False; + if ( last_point < 0.0 ) last_point+=M_2PI; + if ( last_point <= item->arclength ) endstop=False; + else if ( last_point < item->arclength / 2 + M_PI ) endstop=STOPPED_AT_100; + else endstop=STOPPED_AT_0; } break; case wsRLMouseButton:
A comment in the code about the purpose and the logic behind it would be nice ;-) Hans-Dieter
Hans-Dieter Kosch wrote on Tue, 08 Apr 2014 01:08:47 +0200:
It works :-)
I hope this here works even better. It is (except for the documentation) the virtually complete patch for both the X11/GTK and the Win32 GUI. I've tested extensively and it seems to do what it should. There are only two issues left: 1. What delimiter / method should be used for the extra x0, y0, x1, y1? 2. How can floating point and atan2 issues be overcome to determine whether two points x0,y0 and x1,y1 have virtually the same radian? Although I'm going to review the patch by myself taking my time when being well rested, I'd appreciate reviews, tests and reports if there are issues. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Tue, 08 Apr 2014 01:08:47 +0200:
It works :-)
I hope this here works even better.
It is (except for the documentation) the virtually complete patch for both the X11/GTK and the Win32 GUI. I've tested extensively and it seems to do what it should.
There are only two issues left:
1. What delimiter / method should be used for the extra x0, y0, x1, y1?
2. How can floating point and atan2 issues be overcome to determine whether two points x0,y0 and x1,y1 have virtually the same radian?
I'll think about and come back (I've some diffuse ideas), but it may take until next weekend. Hans-Dieter
Hans-Dieter Kosch wrote on Tue, 08 Apr 2014 03:36:06 +0200:
Ingo Brückl wrote:
2. How can floating point and atan2 issues be overcome to determine whether two points x0,y0 and x1,y1 have virtually the same radian?
I'll think about and come back (I've some diffuse ideas), but it may take until next weekend.
It seems that 0.05 (approx. pi/60) is a reasonable threshold even for small images and pixels near the center. Ingo
Ingo Brückl wrote:
2. How can floating point and atan2 issues be overcome to determine whether two points x0,y0 and x1,y1 have virtually the same radian?
It seems that 0.05 (approx. pi/60) is a reasonable threshold even for small images and pixels near the center.
I think, that'd be OK. Or try 1E-8, the epsilon value for a float. Hans-Dieter
I wrote on Tue, 08 Apr 2014 02:44:31 +0200:
1. What delimiter / method should be used for the extra x0, y0, x1, y1?
Of all reasonable characters available for the current method (adding to default value), maybe the slash is the better choice? rpotmeter = NULL, 0, 0, volume, 51, 0/49/52/60/26, 189, 126, 61, 57, evSetVolume Ingo
Ingo Brückl wrote:
1. What delimiter / method should be used for the extra x0, y0, x1, y1?
Of all reasonable characters available for the current method (adding to default value), maybe the slash is the better choice?
rpotmeter = NULL, 0, 0, volume, 51, 0/49/52/60/26, 189, 126, 61, 57, evSetVolume
That looks not bad. But why bind them to the default value and not really separate them by comma, they have logically nothing to do with the default. The syntax of the rpotmeter is different anyway, and the basics for syntax check are now already layed out in the parsing. I'll try to prepare a suggestion. Hans-Dieter
Hans-Dieter Kosch wrote on Thu, 10 Apr 2014 04:11:19 +0200:
Ingo Brückl wrote:
1. What delimiter / method should be used for the extra x0, y0, x1, y1?
Of all reasonable characters available for the current method (adding to default value), maybe the slash is the better choice?
rpotmeter = NULL, 0, 0, volume, 51, 0/49/52/60/26, 189, 126, 61, 57, evSetVolume
That looks not bad. But why bind them to the default value and not really separate them by comma, they have logically nothing to do with the default.
I've come to the same conclusion meanwhile after a snatch of sleep and have already changed it to: rpotmeter = button, bwidth, bheight, phases, numphases, x0, y0, x1, y1, default, X, Y, width, height, message Thanks for persisting anyway. This kind of debate helps finding better solutions. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Thu, 10 Apr 2014 04:11:19 +0200:
Ingo Brückl wrote:
1. What delimiter / method should be used for the extra x0, y0, x1, y1? Of all reasonable characters available for the current method (adding to default value), maybe the slash is the better choice?
rpotmeter = NULL, 0, 0, volume, 51, 0/49/52/60/26, 189, 126, 61, 57, evSetVolume
That looks not bad. But why bind them to the default value and not really separate them by comma, they have logically nothing to do with the default.
I've come to the same conclusion meanwhile after a snatch of sleep and have already changed it to:
rpotmeter = button, bwidth, bheight, phases, numphases, x0, y0, x1, y1, default, X, Y, width, height, message
That looks really good!
Thanks for persisting anyway. This kind of debate helps finding better solutions.
Don't mind, I'm glad that you appreciate :-) MPlayer is my favourite player and open source gives me so much, so I feel for giving back something to the community. Hans-Dieter
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Tue, 08 Apr 2014 01:08:47 +0200:
It works :-)
I hope this here works even better.
Although I'm going to review the patch by myself taking my time when being well rested, I'd appreciate reviews, tests and reports if there are issues.
Seems all fine, no issues detected. One feature request: You made the rpotmeter reacting immediately to the pressing of the mouse button by uiEvent( item->message,100.0 * prev_point / item->arclength ); It would then be nice and consistent to have this feature for the other potmeters, too. It gives in fact more intuitive behaviour on clicking and dragging! Hans-Dieter
Hans-Dieter Kosch wrote on Thu, 10 Apr 2014 03:35:39 +0200:
One feature request: You made the rpotmeter reacting immediately to the pressing of the mouse button by
uiEvent( item->message,100.0 * prev_point / item->arclength );
It would then be nice and consistent to have this feature for the other potmeters, too. It gives in fact more intuitive behaviour on clicking and dragging!
I've actually removed it meanwhile in order to *be* consistent, but it seems that all code from wsRLMouseButton should be moved to wsPLMouseButton. For a simple click, we don't need wsRLMouseButton, and for dragging the value should already have been set by wsMoveMouse. I'll approach it immediately after committing the rpotmeter patch (and it will be the last feature change for a while, because I desperately need a break.) Ingo
Ingo Brückl wrote:
For a simple click, we don't need wsRLMouseButton, and for dragging the value should already have been set by wsMoveMouse.
Exactly, that's also my conclusion.
I'll approach it immediately after committing the rpotmeter patch (and it will be the last feature change for a while, because I desperately need a break.)
Well, it was an exhaustive but successful discussion so far and you committed myriads of reworks meanwhile moreover. If you agree, I'd work out the patch for this feature change, taking you off the work. Hans-Dieter
Hans-Dieter Kosch wrote on Fri, 11 Apr 2014 02:40:58 +0200:
Ingo Brückl wrote:
For a simple click, we don't need wsRLMouseButton, and for dragging the value should already have been set by wsMoveMouse.
Exactly, that's also my conclusion.
I'll approach it immediately after committing the rpotmeter patch (and it will be the last feature change for a while, because I desperately need a break.)
Well, it was an exhaustive but successful discussion so far and you committed myriads of reworks meanwhile moreover.
Yeah, something over 200 since you've proposed "item->pressed = btnReleased" to activate potmeter two month ago (not necessarily all 200 directly related though).
If you agree, I'd work out the patch for this feature change, taking you off the work.
Thank you very much for your kind offer, but I have to confess that I've already prepared the changes last night. I thought that it would be a good idea to already test them along with the rpotmeter patch so that I can commit both on the weekend to have all off the table. (And hopefully there will be no new bugs then.) Ingo
I wrote on Fri, 11 Apr 2014 11:08:36 +0200:
Hans-Dieter Kosch wrote on Fri, 11 Apr 2014 02:40:58 +0200:
Ingo Brückl wrote:
If you agree, I'd work out the patch for this feature change, taking you off the work.
Thank you very much for your kind offer, but I have to confess that I've already prepared the changes last night. I thought that it would be a good idea to already test them along with the rpotmeter patch so that I can commit both on the weekend to have all off the table. (And hopefully there will be no new bugs then.)
Well, and as soon as you try to change a simple feature you're meeting a peck of troubles... When you click on a potmeter button to grab it, it would already move due to its new value now. :-( So it would be necessary to check whether the mouse pointer is inside the button which isn't easily possible at the moment. The best way to realize this would be to store the button's position in render.c to additional members of guiItem in order to be able to access this information in main.c/playbar.c - which, frankly, doesn't seem worth the effort to me, because we'll be running into the next problem for clicks inside the button then, because these kinds of small changes won't be able any longer without having wsRLMouseButton code anyway. Nah! But if you insist, file a trac feature request, but don't expect acceptance of an implementation that will increase code complexity for almost no benefit. Ingo
Ingo Brückl wrote:
Well, and as soon as you try to change a simple feature you're meeting a peck of troubles...
When you click on a potmeter button to grab it, it would already move due to its new value now. :-(
No, yes, of course, that's exactly the pretty behaviour (IMO) :-) Look: 1) Currently: On press nothing happens, on release the value jumps to the new position. 2) Currently: On press nothing happens, on drag the value immediately jumps to the new position. So, if the user presses and then there's a minimal jitter by only 1 px (caused by the user's hand or uneven ground under the mouse), the value jumps to the new position. Such a jitter is surely not noticeable to the user, and has then the same effect like jumping already on press. 3) New (your revoked change): The value would jump already on press, and from there on smoothly follow the drag. This appears to me intuitive and much better than if it would jump surprisingly upon a drag ( 2) ). 4) A mouse click is handled widely different in applications. An action may be taken already on press or not before release (and this intermixed in a single application). So we're free in handling. Our pure button item reacts on release; that's OK and common practice, as we have the chance to drag out of it and nothing happens after release. But with the potmeters, there's always an action on drag or release (which is OK, too); so we can definitely move the release action to a press action. So, I suggest: Stay with your change to move the action from release to press. It's really not trouble, it's an advance. Your last rpotmeter patch worked perfectly regarding that. And the code changes are minimal, I see no risk. What do you think? Hans-Dieter PS: Should you plan to do something during weekend, I'll be closely online and assist in testing.
Hans-Dieter Kosch wrote on Sat, 12 Apr 2014 02:13:21 +0200:
Ingo Brückl wrote:
Well, and as soon as you try to change a simple feature you're meeting a peck of troubles...
When you click on a potmeter button to grab it, it would already move due to its new value now. :-(
No, yes, of course, that's exactly the pretty behaviour (IMO) :-)
For a rpotmeter yes, but I was talking about v/hpotmeters (with a button of some heigth/width) - sorry for forgetting to mention it.
3) New (your revoked change): The value would jump already on press, and from there on smoothly follow the drag. This appears to me intuitive and much better than if it would jump surprisingly upon a drag ( 2) ).
I don't disagree, but in order to prevent a jumping v/hpotmeter button quite some code changes seem necessary then (which certainly will lead immediately to other necessary changes; and it would be useful to revise the ui/*.c code before). The point is, the X11/GTK GUI mouse handler (unlike the Win32 GUI's one) doesn't know and doesn't care about the potmeter buttons. (There are pros and cons either way.)
So, I suggest: Stay with your change to move the action from release to press. It's really not trouble, it's an advance. Your last rpotmeter patch worked perfectly regarding that. And the code changes are minimal, I see no risk.
I'll post my patch after the rpotmeter commit. Get a skin with not too small potmeter buttons (like Clearlooks) and try yourself. You may file a trac feature request then, so that it won't pass out of mind.
PS: Should you plan to do something during weekend,
Yes, my plans are: Release the pending rpotmeter stuff, update and release skins that are using rotary potmeters and take a long (feature) break then spending time with other things again. ;-) Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Sat, 12 Apr 2014 02:13:21 +0200:
Ingo Brückl wrote:
Well, and as soon as you try to change a simple feature you're meeting a peck of troubles...
When you click on a potmeter button to grab it, it would already move due to its new value now. :-(
No, yes, of course, that's exactly the pretty behaviour (IMO) :-)
For a rpotmeter yes, but I was talking about v/hpotmeters (with a button of some heigth/width) - sorry for forgetting to mention it.
The rpotmeter has a button, too, with width/height; I see no difference. render.c already takes care about.
3) New (your revoked change): The value would jump already on press, and from there on smoothly follow the drag. This appears to me intuitive and much better than if it would jump surprisingly upon a drag ( 2) ).
I don't disagree, but in order to prevent a jumping v/hpotmeter button quite some code changes seem necessary then (which certainly will lead immediately to other necessary changes; and it would be useful to revise the ui/*.c code before).
The point is, the X11/GTK GUI mouse handler (unlike the Win32 GUI's one) doesn't know and doesn't care about the potmeter buttons. (There are pros and cons either way.)
It obviously doesn't need to care, it only sets the value. And render.c places the button correctly according to the value, which it does already. More precisely, not the button jumps in first place, but the value does so. So I see no other issues implied. The v/hpotmeter code under wsRLMouseButton would be moved to wsPLMouseButton and all potmeters would have the same behaviour. Hans-Dieter
Hans-Dieter Kosch wrote on Sat, 12 Apr 2014 16:29:25 +0200:
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Sat, 12 Apr 2014 02:13:21 +0200:
Ingo Brückl wrote:
Well, and as soon as you try to change a simple feature you're meeting a peck of troubles...
When you click on a potmeter button to grab it, it would already move due to its new value now. :-(
No, yes, of course, that's exactly the pretty behaviour (IMO) :-)
For a rpotmeter yes, but I was talking about v/hpotmeters (with a button of some heigth/width) - sorry for forgetting to mention it.
The rpotmeter has a button, too, with width/height; I see no difference.
A rpotmeter can have a button, but no rpotmeter you know actually has one! They all have phase images only. Ingo
Ingo Brückl wrote:
Hans-Dieter Kosch wrote on Sun, 16 Feb 2014 23:09:01 +0100:
Attached patch implements a true rotary pot.
Please apply rpotmeter.patch, implement the "/* !!! */" (add code or remove it if not necessary) and post this diff of yours then.
And here the patch for the WIN GUI. Pure theory, untested. Please let me know your findings. Hans-Dieter
On 3/31/2014 5:17 PM, Hans-Dieter Kosch wrote:
And here the patch for the WIN GUI. Pure theory, untested. Please let me know your findings.
Hans-Dieter
I would like to try the WIN GUI patch. What other patches do I need to apply?
Stephen Sheldon wrote on Mon, 31 Mar 2014 17:42:21 -0700:
On 3/31/2014 5:17 PM, Hans-Dieter Kosch wrote:
And here the patch for the WIN GUI. Pure theory, untested. Please let me know your findings.
I would like to try the WIN GUI patch. What other patches do I need to apply?
Besides Hans-Dieter's patch you'll need the attached patch to load a rpotmeter from a skin file, and a suitable skin - you can patch 'standard' or 'phony'. Unfortunately, some skins seem to crash with the Win32 port of the GUI, because there is no error detection resp. no appropriate error handling for some cases during image loading. Actually, both of the mentioned skins are crashing for me under Wine (in pngRead() AFAIR). I haven't yet had time to investigate further. If they do crash for you too, you'll maybe find an other skin to test. You get the idea for a potmeter to rpotmeter redefinition for rotary skin elements from the skin patch. Ingo
Stephen, which of the following skins crash on real Windows (and which don't): disappearer mentalic mplayer_red neutron phony plastic slim softgrip standard trium Please try (starting from a console window) without the rpotmeter patch, click a little bit and then try to exit the GUI. Ingo
participants (3)
-
Hans-Dieter Kosch -
Ingo Brückl -
Stephen Sheldon