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

Add finer zoom, 12.5% #2517

Merged
merged 1 commit into from May 31, 2016
Merged

Add finer zoom, 12.5% #2517

merged 1 commit into from May 31, 2016

Conversation

@zonkmachine
Copy link
Member

zonkmachine commented Jan 26, 2016

Closes issue #2

I added an extra 12% in front of the setup loop. I did this in three places for the Song Editor, Piano Roll and the Automation Editor. The Automation Editor has a second zoom model that I a. didn't grasp, b. failed to test even unmodified... the other (left) is changed though and works fine.

@zonkmachine zonkmachine changed the title Add finer extra zoom, 12% Add finer zoom, 12% Jan 26, 2016
@tresf
Copy link
Member

tresf commented Jan 26, 2016

@zonkmachine why is this closed?

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 26, 2016

why is this closed?

I couldn't stand the defeat... I'm back in the saddle though.

@zonkmachine zonkmachine changed the title Add finer zoom, 12% [WIP] Add finer zoom, 12% Jan 26, 2016
@zonkmachine zonkmachine reopened this Jan 26, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 26, 2016

The selected zoom and the real differs a bit and this becomes a bit brutal on 12%
I believe the rounding off in action is done here:
https://github.com/LMMS/lmms/blob/master/src/gui/editors/SongEditor.cpp#L598:L606
There is integer math in there with truncation involved. Maybe it's not a problem though. If we pick 10% for instance, the visual end zoom is the same but more expected with the lower value. You'd only need this once in a while if you work on longer songs. at 140 bpm and 4/4 beat this covers 35 minutes on my screen while the minimum setting today, 25%, is ~9 minutes.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 26, 2016

@unfa is one of the only artists here who makes tracks this long and maybe would like to have a say in this.

@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 27, 2016

Here's how the math goes:

DEFAULT_PIXELS_PER_TACT = 16

setPixelsPerTact( zfac.left( zfac.length() - 1 ).toInt() *
                    DEFAULT_PIXELS_PER_TACT / 100 );

zoom

12 * 16 / 100 = 1.92 <-- Truncated to 1!
25 * 16 / 100 = 4
50 * 16 / 100 = 8
100 * 16 / 100 = 16

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch 3 times, most recently from bb7c5ff to 5182485 Jan 27, 2016
@zonkmachine zonkmachine changed the title [WIP] Add finer zoom, 12% [WIP] Add finer zoom, 13% Jan 27, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Jan 27, 2016

The end result of13% is truncated to 2 which is equivalent to 12.5% and on target.

@zonkmachine zonkmachine changed the title [WIP] Add finer zoom, 13% Add finer zoom, 13% Feb 7, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 7, 2016

This now works if people are cool with the odd value 13%.

@Umcaruje
Copy link
Member

Umcaruje commented Feb 8, 2016

This now works if people are cool with the odd value 13%.

Could we use 12,5% or would that be too much of a problem?

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 8, 2016

Could we use 12,5% or would that be too much of a problem?

I don't think it would be a whole lot of problem. You would have to test explicitly for that string and substitute for 13 or something that works. I'll try it later...

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 9, 2016

It looks better in the menu. The background grid in the Piano Roll is a bit too much on 12,5%. Like a haze.

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch from 6b7d0b9 to 4d01cd5 Feb 9, 2016
}
int pixelsPerTact = zoom.toInt() * DEFAULT_PIXELS_PER_TACT / 100;

assert( pixelsPerTact > 0 );

This comment has been minimized.

@zonkmachine

zonkmachine Feb 9, 2016 Author Member

I added the assert just to mirror the PianoRoll.cpp and AutomationEditor.cpp .
As the only variable is the zooming factor I think there is no risk of reaching 0 unless you go down to ~5% zoom.

@grejppi
Copy link
Contributor

grejppi commented Feb 9, 2016

English uses a decimal point, not a comma. EDIT: ninja'd

Also, would it be possible to avoid hard-coding, comparing, and parsing strings?

Something like...

// in a header file somewhere
const int NUM_ZOOM_LEVELS = 7;
const float ZOOM_LEVELS[] = {
    8.0f, 4.0f, 2.0f, 1.0f, 0.5f, 0.25f, 0.125f
};
// in the constructor
for (int i = 0; i < NUM_ZOOM_LEVELS; ++i)
{
    m_zoomingModel->addItem(QString("%1\%").arg(ZOOM_LEVELS[i] * 100));
}
// in zoomingChanged()
setPixelsPerTact(ZOOM_LEVELS[m_zoomingModel->value()]);

(Disclaimer: I have not tested the above snippets of code nor have any idea if they're correct, but the general idea should be there.)

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch 2 times, most recently from e2829db to 4eb8858 Feb 9, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 9, 2016

English uses a decimal point, not a comma. EDIT: ninja'd

I ninja'd it too... :(

@grejppi
Copy link
Contributor

grejppi commented Feb 9, 2016

PS: about my comment above, I've edited the code snippets in it a couple of times for obvious logic errors (I really should be sleeping right now!), but I think I'm finished now...

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 9, 2016

Something like...

Nein! We go .toFloat() on all of them strings, calculate like hell and then back to int...

@zonkmachine zonkmachine changed the title Add finer zoom, 13% Add finer zoom, 12.5% Feb 9, 2016
@michaelgregorius
Copy link
Contributor

michaelgregorius commented Feb 9, 2016

I agree with @grejppi that using floats instead of QStrings would look much better in the code. I also like in his example code that the conversion to QString is only done for the combo boxes. In all other places it should not be needed.

However, I would change the zoom levels data structure to an std::vector:

const std::vector<float> m_zoomLevels = { 8.0f, 4.0f, 2.0f, 1.0f, 0.5f, 0.25f, 0.125f };

This way you would not need to adjust an extra variable like NUM_ZOOM_LEVELS in case you add more zoom levels because at all the other places in the code you would simply use m_zoomLevels.size() to get the number of zoom levels or even better use the new for loops provided by C++11:

for (float const & zoomLevel : m_zoomLevels)
{
    m_zoomingModel->addItem(QString("%1\%").arg(zoomLevel * 100));
}

Last but not least I'd like to add that I think that 16 pixels per tact is too small anyway. In my opinion the default zoom level of the song editor does not really communicate "Here is a broad canvas to realize your musical ideas!". I also find it "unnatural" that the height of one tact is bigger than it's width. The reason is that musical notation expands from the left to the right so more space should be provided in that dimension.

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 9, 2016

You're obviously right, both of you, and it cleans up the code like nothing else. Trying it out now.

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 10, 2016

Got an ouchie!
This was on the linux builds. Mac and Win passed. I'm on Ubuntu/Mint and didn't see any complaints.

/home/travis/build/LMMS/lmms/include/SongEditor.h:127:4: error: a brace-enclosed initializer is not allowed here before ‘{’ token

/home/travis/build/LMMS/lmms/include/SongEditor.h:127:57: sorry, unimplemented: non-static data member initializers

/home/travis/build/LMMS/lmms/include/SongEditor.h:127:57: error: in-class initialization of static data member ‘m_zoomLevels’ of non-literal type
@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch from 7be83e4 to f6ebadd Feb 10, 2016
@grejppi
Copy link
Contributor

grejppi commented Feb 10, 2016

@zonkmachine @michaelgregorius code in LMMS tends to prefer QVector and friends over the STL. Nothing big, just a consistency thing.

You might want to consider making the vector static:

// Class.h

class Class {
    // ...
    static const QVector<float> s_zoomLevels;
    // ...
};
// Class.cpp

const QVector<float> Class::s_zoomLevels({ 8.0f, 4.0f, 2.0f, 1.0f, 0.5f, 0.25f, 0.125f });
// ...
@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 10, 2016

You might want to consider making the vector static:

That was pretty much how I spent my night. ;-)
I changed to a QVector but I keep getting the same messages from the linker in the end:

CMakeFiles/lmmsobjs.dir/core/Song.cpp.o:(.bss+0x8): multiple definition of `PianoRoll::m_zoomLevels'
CMakeFiles/lmmsobjs.dir/core/Mixer.cpp.o:(.bss+0x0): first defined here
CMakeFiles/lmmsobjs.dir/core/Song.cpp.o:(.bss+0x0): multiple definition of `SongEditor::m_zoomLevels'
CMakeFiles/lmmsobjs.dir/core/DataFile.cpp.o:(.bss+0x80): first defined here
...
@tresf
Copy link
Member

tresf commented Feb 10, 2016

I changed to a QVector but I keep getting the same messages from the linker in the end:

Well, that's quite a different message I think... In this case, AutomationEditor::m_zoomXLevels and m_zoomXLevels are declared twice in the same scope... one as a private variable, one as a signal, no?

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 10, 2016

one as a signal,

No, it's after the signals outside of the class.

@tresf
Copy link
Member

tresf commented Feb 10, 2016

No, it's after the signals outside of the class.

Then we must be looking at different code. m_zoomXLevels seems odd as a signal anyway, is there a reason why you have it there?

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch 2 times, most recently from 9ff261b to 3695eba Feb 10, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 10, 2016

Then we must be looking at different code. m_zoomXLevels seems odd as a signal anyway, is there a reason why you have it there?

There was a commented out initialisationtion statement in the class and there was a wrongly indented definition of m_zoomXLevels but it was outside of the Class declaration, after the 'signals'.
That's changed anyway.

Initiations moved to .cpp files

A secret society just got booked horribly wrong...

@Umcaruje
Copy link
Member

Umcaruje commented Feb 15, 2016

@zonkmachine I tested it out, but noticed a bug:
gifrecord_2016-02-15_014232

When +scrolling to zoom in/out in the song editor, it only goes down to 25%, but not on 12.5%. When I scroll over the combo box, this appeares not to be an issue.

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 15, 2016

Thanks, so that's what the SongEditor::wheelEvent does!
Back to the dungeon for some deep research...

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 15, 2016

I stealthily copied over the solution from PianoRoll.cpp
Thanks for the review.

@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 16, 2016

Mouse wheel zoom and combo box zoom now works in opposite directions...

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch from 1052a7e to cfd20b7 Feb 16, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented Feb 25, 2016

Mouse wheel zoom and combo box zoom now works in opposite directions...

When you set zooming by scrolling the combo box or by Ctrl+Scrolling the track the wheel directions aren't the same. I thought I messed that one up but it's actually there in master - Fixed

@Umcaruje
Copy link
Member

Umcaruje commented May 18, 2016

@zonkmachine could you please rebase so we get this PR merged?

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch 2 times, most recently from 293fd03 to 5c945f8 May 18, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented May 18, 2016

Rebased and tested.

@zonkmachine zonkmachine force-pushed the zonkmachine:zoom branch from 5c945f8 to 36c181d May 28, 2016
@Umcaruje
Copy link
Member

Umcaruje commented May 31, 2016

Tested out, works perfectly, @grejppi gave the code an ok, code looks good to me as well.
Merging 🎉

@Umcaruje Umcaruje merged commit 9c5cbbe into LMMS:master May 31, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zonkmachine zonkmachine mentioned this pull request May 31, 2016
@zonkmachine
Copy link
Member Author

zonkmachine commented May 31, 2016

@Umcaruje Thanks for review, merge and keeping the spirit up...

@zonkmachine zonkmachine deleted the zonkmachine:zoom branch Jun 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.