Simplify zoom and snap calculations, remove TextFloat #1
Conversation
| @@ -314,8 +314,7 @@ void ClipView::updateLength() | |||
| } | |||
| else | |||
| { | |||
| // 3 is the minimun width needed to paint a clip | |||
There was a problem hiding this comment.
Technically the border width is 2 pixels so minimum width of a clip would be 5. But it works even if pixelsPerBar is 1. So I don't see a need to enforce a minimum here... Just don't let zoom get to small. Or am I missing something?
There was a problem hiding this comment.
That was needed for clips that do not start or end at beat times (Alt+mouse, to move start or end of the clip) and an "extreme" zoom in (clip disappears). It happens with your code as well. I think we should reintroduce that control again.
| constexpr int MAX_PIXELS_PER_BAR = 400; | ||
| constexpr int ZOOM_STEPS = 200; | ||
|
|
||
| constexpr std::array SNAP_SIZES{8.f, 4.f, 2.f, 1.f, 1/2.f, 1/4.f, 1/8.f, 1/16.f}; |
There was a problem hiding this comment.
I opted to define snap sizes manually instead of calculating them on three different places. This opens the possibility of snap sizes like 1/3 etc
| } | ||
| constexpr int MIN_PIXELS_PER_BAR = 2; | ||
| constexpr int MAX_PIXELS_PER_BAR = 400; | ||
| constexpr int ZOOM_STEPS = 200; |
There was a problem hiding this comment.
ZOOM_STEPS is the number of possible steps in the slider. Due to the logarithmics this value doesn't mean anything special. It just affects the speed of zoom, since scrolling moves the slider by one step. So scrolling 200 steps goes from min to max zoom.
| { | ||
| // log: val should be between [0..1] | ||
| float normVal = (static_cast<float>(val) - ZOOM_MIN) / (ZOOM_MAX - ZOOM_MIN); | ||
| float logVal = logToLinearScale(ZOOM_MIN, ZOOM_MAX, normVal); |
There was a problem hiding this comment.
(my brain was burning up trying to understand scaleToLogZoom and logToLinearScale) 🤣
There was a problem hiding this comment.
😄 Me too now that I'm reviewing my own code 😱
| @@ -142,8 +143,7 @@ private slots: | |||
|
|
|||
| PositionLine * m_positionLine; | |||
|
|
|||
| IntModel* m_zoomingLinearModel; | |||
| int m_zoomingLogValue; | |||
| IntModel* m_zoomingModel; | |||
There was a problem hiding this comment.
I renamed it back to zoomingModel, since there is only one now.
| val = val - static_cast<int>(round(log2(m_zoomingLogValue / ZOOM_MIN))) + 3; | ||
| // Finds the closest available snap size | ||
| const float optimalSize = snapSize * DEFAULT_PIXELS_PER_BAR / pixelsPerBar(); | ||
| return *std::min_element(SNAP_SIZES.begin(), SNAP_SIZES.end(), [&](float a, float b) |
There was a problem hiding this comment.
It's not pretty but it would support snap sizes that aren't powers of 2.... (Or if we leave out snap sizes like "2 bars")
| if (m_iniBar != newBar) | ||
| { | ||
| m_leftRightScroll->setValue(m_leftRightScroll->value() + m_iniBar - newBar); | ||
| m_iniBar = newBar; |
There was a problem hiding this comment.
Since you write to m_iniBar every time it moves to another bar, it serves no purpose any longer.
| realignTracks(); | ||
| updateRubberband(); | ||
| m_timeLine->setSnapSize(getSnapSize()); | ||
|
|
||
| emit zoomingValueChanged(m_zoomingLogValue / 100.0f); |
There was a problem hiding this comment.
Since I removed all use of percentage zoom in the calculations, I did that for this signal too
| m_zoomingSlider->setOrientation(Qt::Horizontal); | ||
| m_zoomingSlider->setPageStep(1); | ||
| m_zoomingSlider->setFocusPolicy(Qt::NoFocus); |
There was a problem hiding this comment.
The focused widgets receive scroll events first. If they ignore the event, it will be sent to the widget under the cursor. So if the slider gets focus by clicking it you won't be able to ctrl+scroll.
|
|
||
| m_editor->m_zoomingLogValue =newVal; |
There was a problem hiding this comment.
As you may have seen by now, we don't store percentage values anywhere. The slider value is just some arbitrary linear scale that affects the precision of zoom. And for everything else we use pixelsPerBar(). Since a song doesn't have a "physical width" the use of 100% zoom is meaningless. It's not like a document that has a physical size on paper. Therefore I opted to remove the float, and let the user zoom to the desired level without knowing the exact value. This saves us some code and saves OCD people some anxiety over how hard it is to hit exactly 100% with the slider 😜
| for( int i = 0; x + i * m_ppb < width(); ++i ) | ||
| { | ||
| ++barNumber; | ||
| // round m_ppb to the nearest power of two | ||
| const float ppbP2 = pow(2, round(log2(m_ppb))); |
There was a problem hiding this comment.
I must say your calculation here is really clever, but I tried to make it a bit more understandable for a math noob like me.
|
I see very clever changes here! I'm going to merge it. I never done this before. I hope to do it alright ;-) |
Here are some suggested changes to make the zoom logic more easy to understand. I also removed the TextFloat... I'll explain why in the comments. If you decide it's ok you can merge it, otherwise you may handpick the parts you like.