From: Yves G. <yc....@wa...> - 2013-07-18 20:36:41
|
It seems that my last mail is still waiting moderator... So I send it again but without the screen shots which can be found here: http://sevy.tom.pagesperso-orange.fr/rosegarden/test/screens.html Le mardi, juillet 16, 2013 à 03:01:51 AM, D. Michael McIntyre a écrit : >On 07/15/2013 07:21 PM, Tom Breton (Tehom) wrote: >> They sound very serious, but I can't see them. > >Nor can I. None of them. > >How clean is your build, Yves? I know I did a distclean and rebuild >rather recently. Sorry for that noise : after a make clean and a rebuild, the pitch bend ruler is back and works fine. Nevertheless I still have a problem related to volume controllers insertion. Here is how I proceed: - Open an example (there is only 3 examples left now ?) Well, I opened Chopin Prelude in E minor. - Open Track 3 in notation - View -> Rulers -> Add Control Ruler -> Volume Controller - Select all notes inside bar 4 [screen1.png] - Controllers -> Insert Controllers Sequence Replacement mode: Replace old events Preset: Saved setting 1 Ramp duration (%): 100.00 End value: 0 Start amplitude: 0.00 End Amplitude: 0.00 Hertz (Hz): 10.00 Ramp Mode: Logarithmic How many steps: Use this many steps: 40 [screen2.png] - Undo the last operation (it works) and close the notation window - Open Track 3 in the matrix - View -> Rulers -> Add Control Ruler -> Volume Controller - Select all notes inside bar 4 again [screen3.png] - Controllers -> Insert Controllers Sequence (keep the same settings) [screen4.png] - close the matrix and open Track 4 in notation [screen5.png] I'm running KDE 4.6.5 Rosegarden version 13.06 ("Imagination") Build key: f733dbe9a6 Qt version: 4.7.4 Of course there is not the last Qt version, but I can't imagine it really makes a difference. Yves |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-20 15:58:28
|
> > After an upgrade to Qt 4.8.5 I didn't see any significant difference. > (Only one difference: no more black rectangle in the matrix.) > > Le samedi, juillet 20, 2013 � 03:39:36 AM, Tom Breton (Tehom) a �crit : >>> Le vendredi, juillet 19, 2013 � 07:04:48 PM, Tom Breton (Tehom) >>> a > >>So I wondered if maybe Logarithmic was calculating crazy timeRatios and >>therefore making us scribble events outside the proper time interval. >>That shouldn't screw up the display like that, though. It doesn't for >>other insert-event-at-crazy-time bugs. > > Looking in the code, I discovered the problem was indeed here. I'm cc-ing Tim Munro. That bit of code (the Logarithmic calculations) is from his patch so he should be in the loop. > When ramp start value end end value are the same, the timeRation computed > is a nan (0/0) (at > least on my system) and all the events are inserted at time > -9223372036854775808 which gave > the strange GUI behaviour I observed. > > I have no idea why I was the only one to see the problem. Possibly your compiler handles floating point or NaN differently? > I just commited a fix which seems to work here (with Qt 4.7.4 as with Qt > 4.8.5). Thank you. It changes the other branches' logic a little. If you don't mind, I'm going to write a slightly different fix. > I didn't test it extensively. So thanks to verify I didn't break anything. I will. Tom Breton (Tehom) |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-20 17:04:51
|
First, I forgot to thank you, Yves, for finding that bug. Thank you! On my system, I still can't make it happen. I've written a different fix that keeps the zero denominator condition from happening at all rather than tweaking the calculation. I check it with an assert, so if it somehow happens again everybody will see assertfail immediately; you won't have to dig it out of weird display issues and divergent platforms again. I got a little carried away so now it checks for a number of other conditions that might feed unexpected values to the calculations or just create redundant events. I'm testing it now. So far it looks good. Tom Breton (Tehom) |
From: Ted F. <te...@te...> - 2013-07-20 18:37:38
|
On 07/20/2013 01:04 PM, Tom Breton (Tehom) wrote: > I check it with an assert, ISTM that rosegarden avoids assertions as much as possible, and instead avoids situations that might cause them. I assume this is related to not wanting to lose the users data if they haven't saved recently. E.g. if (p) p->foo(); ...instead of assert(p); p->foo(); If you search the code for assert you will find assertions are very rarely used. Also, note where they are used the most. If you must assert, I think it would be best to use Q_ASSERT_X() which provides additional information to help with tracking down the cause of an assertion. If anyone can elaborate on what the rosegarden philosophy has been WRT assertions, that would be helpful. Adding it to the standards on the wiki would be great too. Ted. |
From: D. M. M. <ros...@gm...> - 2013-07-20 20:37:03
|
> If anyone can elaborate on what the rosegarden philosophy has been > WRT assertions, that would be helpful. Adding it to the standards on > the wiki would be great too. I can't elaborate, I just remember being on the wrong end of a sharp tongue over it once. You've got the spirit of it right, I gauge. -- D. Michael McIntyre |
From: Yves G. <yc....@wa...> - 2013-07-20 20:41:40
|
Le samedi, juillet 20, 2013 à 07:04:45 PM, Tom Breton (Tehom) a écrit : >First, I forgot to thank you, Yves, for finding that bug. Thank you! On >my system, I still can't make it happen. You are welcome Tom. On my system, the problem was obvious, even if I was unable to find out its origin at first. >I've written a different fix that keeps the zero denominator condition >from happening at all rather than tweaking the calculation. > >I check it with an assert, so if it somehow happens again everybody will >see assertfail immediately; you won't have to dig it out of weird display >issues and divergent platforms again. > >I got a little carried away so now it checks for a number of other >conditions that might feed unexpected values to the calculations or just >create redundant events. > >I'm testing it now. So far it looks good. I just did some tests with your fix and there is no more problem here. Nevertheless something is curious : when the ramp is logarithmic, the "use this many steps" value doesn't seem to be used. The number of inserted controllers depends of the height of the ramp. This behaviour is different when the ramp is linear. I understand that the idea is to only insert useful controllers, ignoring redundant ones, but why only on logarithmic ramps and not on the linear ones ? Moreover, the user may be surprised to not found the number of controllers he asked for. Nothing in the GUI says the entered value is a maximum but the tooltip says "The sequence will have _exactly_ that many steps". Yves |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-20 21:26:24
|
>>I'm testing it now. So far it looks good. > > I just did some tests with your fix and there is no more problem here. Excellent! > Nevertheless something is curious : when the ramp is logarithmic, the "use > this many steps" value doesn't seem to be used. It is still used, but now a later test skips adding events that are exactly the same as the last controller event - they would accomplish nothing. If you want to see, try it with a large ramp. > This behaviour is different when the ramp is linear. > > I understand that the idea is to only insert useful controllers, ignoring > redundant ones, but why only on logarithmic ramps and not on the > linear ones ? Because the linear ramp + `Use this many steps' might also incorporate vibrato. That is trickier to calculate and lives in a separate method. For that, I can't sensibly skip values equal to the start or end value. If that value was reached due to a vibrato cycle, its event would be skipped, which would be wrong. For consistency, I could add a check for redundant values equal to the previous value. > Moreover, the user may be surprised to not found the number of controllers > he asked for. > Nothing in the GUI says the entered value is a maximum but the tooltip > says "The sequence will have _exactly_ that many steps". That is true. I don't want to surprise the user, but it's also a little surprising to add events that do nothing, and the user probably didn't mean to ask for that. The options, as I see them: * Leave it the way it is. I'd rather solve it then let it confuse people, though. * Remove that check. Then we'll always insert the exact number of steps even if it's probably a mistake. * Document it differently - yet again. Based on the "Oh, not again" I just imagined coming from the direction of VA, probably not. * Constrain the dialog: don't let "Use this many steps" exceed the number of steps available. What do you think? Tom Breton (Tehom) |
From: Yves G. <yc....@wa...> - 2013-07-21 15:07:11
|
Le samedi, juillet 20, 2013 à 11:26:16 PM, Tom Breton (Tehom) a écrit : >The options, as I see them: > * Leave it the way it is. I'd rather solve it then let it confuse >people, though. > * Remove that check. Then we'll always insert the exact number of steps >even if it's probably a mistake. > * Document it differently - yet again. Based on the "Oh, not again" I >just imagined coming from the direction of VA, probably not. > * Constrain the dialog: don't let "Use this many steps" exceed the number >of steps available. > >What do you think? My preference would be to add a check box to remove or not the redundant events. But this is only because I like to control everything and be able to do exactly what I want, even if it is something stupid :) I presume that if a RG release is coming soon, the wisest thing to do is to leave it the way it is. Yves |
From: D. M. M. <ros...@gm...> - 2013-07-20 21:55:57
|
> That is true. I don't want to surprise the user, but it's also a little > surprising to add events that do nothing, and the user probably didn't > mean to ask for that. Behavior sounds reasonable. The click then ctrl+click method of inserting a line of controllers on the ruler has caused similar confusion by avoiding redundant values, but people got over it when they realized why their ramp was a big wide stairstep. How far away from reality is the text? How hard to change the text to reflect reality? I hate to make new work for translators at this time. Actually, let's just leave the text alone and go with it like it is for now. One of these days I'm probably going to get motivated to put a writer's hand to a lot of the stuff you wrote anyway. There are a lot of vague things that just don't quite sit right, but dressing up the texts was more of a project than I wanted at the time I considered it. More important to get the release out than to get every detail perfect. -- D. Michael McIntyre |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-20 23:05:23
|
> How far away from reality is the text? How hard to change the text to > reflect reality? I hate to make new work for translators at this time. It's somewhat misleading, but fixing it seems potentially tricky; if my normal stuff doesn't sit right, I can imagine how my explanation of exactly how it's not always exact would sit. I hate to make new work for you/them too. I try to consider how my work affects that. > Actually, let's just leave the text alone and go with it like it is for > now. Yes, let's leave the text alone. There are at least 3 options that don't involve changing it. > One of these days I'm probably going to get motivated to put a > writer's hand to a lot of the stuff you wrote anyway. There are a lot > of vague things that just don't quite sit right, but dressing up the > texts was more of a project than I wanted at the time I considered it. Feel free to. I know that my text isn't always as clear as it could be. Tom Breton (Tehom) |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-20 23:19:44
|
> On 07/20/2013 01:04 PM, Tom Breton (Tehom) wrote: >> I check it with an assert, > > ISTM that rosegarden avoids assertions as much as possible, and > instead avoids situations that might cause them. It's a case that (now) shouldn't be logically possible. IMHO it's better for a failure of this logic to be an immediately obvious failure, but I will of course conform to team standards. > If you must assert, I think it would be best to use Q_ASSERT_X() > which provides additional information to help with tracking down the > cause of an assertion. I did not know that QT had that. Thank you. Tom Breton (Tehom) |
From: Ted F. <te...@te...> - 2013-07-21 03:10:34
|
On 07/20/2013 07:19 PM, Tom Breton (Tehom) wrote: > It's a case that (now) shouldn't be logically possible. IMHO it's better > for a failure of this logic to be an immediately obvious failure, but I > will of course conform to team standards. I'm not sure what those "standards" are, so don't conform yet. We might need to set the standards now. Assertions only fire with a debug build, so preventing people from losing work is probably not that big of a deal. If they are running a debug build, they should know better (save frequently). I think the bigger picture issue is that oftentimes code assumes that once the assertion fires, the program ends. I think we need to at least take into account the fact that for a non-debug build, the assertion will not happen, and therefore we should be careful not to crash on the user. And I think that's the real spirit of what I commonly see throughout rg: if (p) p->foo(); With this, we do no harm in a debug or a non-debug build if p happens to be 0. Achieving the same with an assert is as simple as adding the assert: Q_ASSERT_X(p, "bar()", "p shouldn't be 0"); if (p) p->foo(); Of course, each case will be different. But the idea is to handle things cleanly without the assertion. Then add the assertion if you really want to crash on users of a debug build. We should use something like this as a starting point for a rosegarden assertion guideline. I think handling problems cleanly in debug and non-debug builds is easy to agree on. Whether assertions should be allowed might be a bit harder to agree on. On the upside, assertions do grab our attention so that we address the issue. On the downside, assertions are crashes that cause data loss, but only in a debug build. Is anyone seriously morally opposed to this? Ted. |
From: D. M. M. <ros...@gm...> - 2013-07-21 13:35:39
|
> On the downside, assertions are crashes that cause data loss, but > only in a debug build. Is anyone seriously morally opposed to this? Nope. Nothing gets your attention like a boom accompanied by an assert. I like asserts. I have no idea why Rosegarden avoids them. I always use and do all real creative work with a debug build, and I've had more crashes than I could count. I've lost some work. It comes with the territory. -- D. Michael McIntyre |
From: D. M. M. <ros...@gm...> - 2013-07-21 13:55:19
|
> Yes, let's leave the text alone. There are at least 3 options that don't > involve changing it. We'll deal with all of that at a later date then. > Feel free to. I know that my text isn't always as clear as it could be. You remind me a lot of the guy who built this thing: http://www.youtube.com/watch?v=p9H-p_6OJPo He was unhappy with the synth software of the day, so damn it, he built a synth that did exactly what he wanted. He had a lot of obscure problems with that thing, and it turned out he wasn't using MIDI cables. He just had five stripped wires poked into the appropriate holes at either end. I guess the moral of the story is that really smart people are weird. I'm smarter than a lot of people in my life, and they're always telling me how weird I am. You're smarter than me, and here we are. -- D. Michael McIntyre |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-21 16:19:30
|
> > You remind me a lot of the guy who built this thing: > > http://www.youtube.com/watch?v=p9H-p_6OJPo That is hilarious! Amazing that he got it to work. Tom Breton (Tehom) |
From: Ted F. <te...@te...> - 2013-07-21 17:07:59
|
On 07/21/2013 09:55 AM, D. Michael McIntyre wrote: > http://www.youtube.com/watch?v=p9H-p_6OJPo That was inspirational to say the least. I think we need a video link collection on the wiki. Ted. |
From: D. M. M. <ros...@gm...> - 2013-07-21 23:30:52
|
>> http://www.youtube.com/watch?v=p9H-p_6OJPo > > That was inspirational to say the least. In a way, I think he inspired the trebuchet I built out of an end table, a piece of railroad track, bits of a Nordic Trac, and so forth. It worked. Sort of. > I think we need a video link collection on the wiki. Why not. -- D. Michael McIntyre |
From: D. M. M. <ros...@gm...> - 2013-07-22 02:48:41
|
Hmmmm... Messing with the pitch bend sequence dialog to test translations, it's really apparent it needs some show() hide() whatzits to make it obvious that this and that only work then that and this are in use, &c. Not happening for 13.06, which is already tagged and bagged. Release in progress. -- D. Michael McIntyre |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-22 15:37:41
|
> Hmmmm... > > Messing with the pitch bend sequence dialog to test translations, it's > really apparent it needs some show() hide() whatzits to make it obvious > that this and that only work then that and this are in use, &c. OK. They're enabled/disabled now and they shade the little arrows to show it, but it shouldn't be hard to make them show/hide instead. Tom Breton (Tehom) |
From: Tom B. (Tehom) <te...@pa...> - 2013-07-22 17:17:01
|
> > OK. They're enabled/disabled now and they shade the little arrows to show > it, but it shouldn't be hard to make them show/hide instead. I take that back. I just remembered why I wrote the comment "show()/hide() on m_vibratoBox is buggy so we don't use it." That was why I enabled/disabled the fields instead. I can hide just the fields easily enough. Then the dialog looks like a skeleton: The boxes and labels are there but the fields are gone. It gets the point across but maybe at too high a cost. What I can't do is hide / show the groupboxes such as m_vibratoBox. That crashes. It may be fixed in later versions of Qt, but I don't think we should introduce a version dependency just for this. Tom Breton (Tehom) |
From: D. M. M. <ros...@gm...> - 2013-07-22 19:10:12
|
> It may be fixed in later versions of Qt, but I don't think we should > introduce a version dependency just for this. We show/hide a million other things, so I bet there's still a way to make it work, using a slightly different approach or something. It's not a big deal, it's just the kind of pointless fiddly polish and fine tuning that I'm good at, and I should take it as a cue to step up and do my damn job. You guys write hard code that does amazing things, and I make it look prettier on the surface. I'm really kind of useless for anything else when you get down to it. Especially lately. You shouldn't take any of these running comments as a TODO list for yourself. Think of them as notes to me that I may or may not get around to acting on one day. I might. I ended up finishing that entire translation, for example. -- D. Michael McIntyre |
From: Ted F. <te...@te...> - 2013-08-01 14:03:21
|
On 07/21/2013 09:35 AM, D. Michael McIntyre wrote: >> On the downside, assertions are crashes that cause data loss, but >> only in a debug build. Is anyone seriously morally opposed to this? > Nope. Ok, since no one has argued against allowing assertions in rosegarden, it appears that they are now "officially" allowed. I've added some guidance to the coding standards on the wiki: http://www.rosegardenmusic.com/wiki/dev:coding_style#assertions So, Tom, assert away. Ted. |
From: Tom B. (Tehom) <te...@pa...> - 2013-08-02 02:15:05
|
> I've added some guidance to the coding standards on the wiki: > > http://www.rosegardenmusic.com/wiki/dev:coding_style#assertions > > So, Tom, assert away. I've just rewritten our assertions to qt style. Those that I understood (and wrote), I used Q_ASSERT_X for; those that I didn't know, I changed to Q_ASSERT. Tom Breton (Tehom) |
From: Tom B. (Tehom) <te...@pa...> - 2013-08-01 17:44:51
|
> On 07/21/2013 09:35 AM, D. Michael McIntyre wrote: >>> On the downside, assertions are crashes that cause data loss, but >>> only in a debug build. Is anyone seriously morally opposed to this? >> Nope. > > Ok, since no one has argued against allowing assertions in > rosegarden, it appears that they are now "officially" allowed. OK. Thank you for following up on this. > I've added some guidance to the coding standards on the wiki: > > http://www.rosegardenmusic.com/wiki/dev:coding_style#assertions I have reservations about trying to recover when NDEBUG isn't defined. That itself is coding and debugging effort. ISTM it makes more sense to spend all of that effort on the underlying problem, so there's nothing to recover from. This is what we do now and it works. Coding effort is probably better spent on conditions that aren't thought to be impossible. But it brings up something I've been meaning to mention. I'm *not* proposing that we do this any time soon. I'm just throwing it out there as an idea. Let's say we do try to recover in commands, from should-have-been asserts or just from runtime problems, bad parameters, etc. After "if (p)" you still have to whole problem of how to actually recover. Right now, you have to execute some zombie version of the command, the user has to figure out what happened and realize he has to undo it, and then your unexecute has to cope with any damage that happened. We might want to consider an approach where a command can fail in "execute" by setting its own concerns back to their original state and raising a specific exception. That exception handler would be responsible for getting back to the previous stable state, other than undoing the specific action of the command. Fleshing it out a bit, that handler would probably live in CommandHistory. It might remove the command from history and explain what happened (delegating to some pop-up message or something). It'd be responsible for making macrocommands recover gracefully. Tom Breton (Tehom) |
From: Ted F. <te...@te...> - 2013-08-01 23:05:02
|
On 08/01/2013 01:44 PM, Tom Breton (Tehom) wrote: > I have reservations about trying to recover when NDEBUG isn't defined. Ok, double-negative. Let me process that for a moment. NDEBUG not defined means we are in... Um.... DEBUG mode! (Did I get that right?) So you are saying that we shouldn't try to recover in a debug build. That's fine and I agree. The recommendation on the wiki is for recovering when NDEBUG *is* defined (a non-debug build). This way in a non-debug build, we never lose the user's data. That is very important. I could definitely see hostility towards assertions if they aren't balanced with proper handling of the problem in the non-debug build. And that might be related to why rg doesn't have many assertions. > But it brings up something I've been meaning to mention. I'm *not* > proposing that we do this any time soon. I'm just throwing it out there > as an idea. Exceptions weren't part of the original design of rg, so it's going to be difficult to work them in. It requires a different mindset. I have absolutely no experience with exceptions as I started with C++ before exceptions were available (1991 or so) or properly understood. It would be really interesting to see the pros and cons of using them on a large project. I have heard quite a bit, but haven't had any hands-on. Ted. |