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

Aliasing in AutomationPatternView and AutomationEditor fixed #3386

Merged
merged 16 commits into from Mar 13, 2017

Conversation

@WalterSmuts
Copy link
Contributor

@WalterSmuts WalterSmuts commented Feb 26, 2017

Exactly the same as this pull request: #3385 but now from a different branch on my side. (Sorry still learning) It's referencing issue #2688

@tresf
Copy link
Member

@tresf tresf commented Feb 26, 2017

Sorry still learning

No need to apologize. There's no easy way to switch off of a master branch, this was the right thing to do. The only portion that may be worth carrying over is any remaining discussion items such as this:

Before:
before
After:
after

"After many hours of trying different things I am quite happy with what happens when the following code changes are applied. It solves the anti-aliasing issue and by drawing polygons instead of rectangles and uses the anti aliasing hint. I also made sure there are no shading inconsistencies.

For some reason however it sometimes doesn't want to draw an automation that is not linked to anything. I believe it is because the values ranges are between 0 and 1 and not 0 and 100 but this is also the case for the master branch and for some reason it works. Can anyone shed some light on it?"

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Feb 26, 2017

For some reason however it sometimes doesn't want to draw an automation that is not linked to anything. I believe it is because the values ranges are between 0 and 1 and not 0 and 100 but this is also the case for the master branch and for some reason it works. Can anyone shed some light on it?

By changing all the QPoints to QPointFs you solve the problem, but a new glitch occurs:
screenshot from 2017-02-26 17 42 14

There's again transparency in the painter paths from the huge amount of transforming and antialiasing that happens here.

Could we try to push all the values into a singular path and then draw that path?

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 26, 2017

Ok awesome! Seems to work now without and shading inconsistencies or leaving out somethings... Now all that needs to be done is the same for the editor itself. @Umcaruje Thanks for pointing out the QPointF thing. It was driving me crazy.

p.fillRect( QRectF( x1, 0.0f, x2 - x1, value ), col );
}
}
path.lineTo(x_base + ((it + 1).key()) * ppt / MidiTime::ticksPerTact(),values[(it + 1).key() - 1 - it.key()]);

This comment has been minimized.

@Umcaruje

Umcaruje Feb 26, 2017
Member

ppt / MidiTime::ticksPerTact() appears a lot of time in the code, you could make it a seperate variable so we don't calculate this all the time.

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 26, 2017

ppt / MidiTime::ticksPerTact() appears a lot of time in the code, you could make it a seperate variable so we don't calculate this all the time.

Done!

@@ -278,6 +278,7 @@ void AutomationPatternView::paintEvent( QPaintEvent * )

const float y_scale = max - min;
const float h = ( height() - 2 * TCO_BORDER_WIDTH ) / y_scale;
const int MidiTpT = MidiTime::ticksPerTact();

This comment has been minimized.

@zonkmachine

zonkmachine Feb 26, 2017
Member

I think what was suggested was to do away with a computation by doing it once, such as:
const int MidiTpT = ppt / MidiTime::ticksPerTact();

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 26, 2017

Done... I was quite silly there

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Feb 26, 2017

Cool, could you also adjust the spaces in parenthesis to be consistent with the rest of the file? I know this is not required anymore, but it really stands out.

Also would you do the same rendering process for the automation editor in this PR? So we could kill two birds with one stone.

@@ -278,6 +278,7 @@ void AutomationPatternView::paintEvent( QPaintEvent * )

const float y_scale = max - min;
const float h = ( height() - 2 * TCO_BORDER_WIDTH ) / y_scale;
const float ppTact = ppt / MidiTime::ticksPerTact();

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

This should actually be named ppTick, cause ppt already means pixels per tact, and you're dividing it with the number of ticks in a tact 🙂

This comment has been minimized.

@WalterSmuts

WalterSmuts Feb 27, 2017
Author Contributor

What is a tact?

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

It's the german/european word for a Bar in music: https://en.wikipedia.org/wiki/Bar_(music)

if( x1 > ( width() - TCO_BORDER_WIDTH ) ) break;
float value = values[ i - it.key() ];
path.lineTo( QPointF( x1,value ) );

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

There should be a space after the comma: x1, value

MidiTime::ticksPerTact();
const float x2 = x_base + (i + 1) * ppt /
MidiTime::ticksPerTact();
const float x1 = x_base + i * ppTact;

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

as there is no x2 now, this variable could be called just x to avoid confusion.

p.fillRect( QRectF( x1, 0.0f, x2 - x1, value ), col );
}
}
path.lineTo( x_base + ( ( it + 1 ).key() ) * ppTact,values[ ( it + 1 ).key() - 1 - it.key() ] );

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

same here, space after comma

}
}
path.lineTo( x_base + ( ( it + 1 ).key() ) * ppTact,values[ ( it + 1 ).key() - 1 - it.key() ] );
path.lineTo( x_base + ( ( it + 1 ).key() ) * ppTact,0.0f );

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

same as previous comment

for( int i = it.key(); i < (it + 1).key(); i++ )

QPainterPath path;
QPointF origin = QPointF(x_base + it.key() * ppTick,0.0f);

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

Missing spaces in parenthesis (sorry I somehow missed this one)

This comment has been minimized.

@WalterSmuts

WalterSmuts Feb 27, 2017
Author Contributor

*( sorry I somehow missed this one )

@WalterSmuts WalterSmuts force-pushed the WalterSmuts:Anti-Aliasing branch from 1538466 to af9ed17 Feb 27, 2017
@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 27, 2017

Ok so I managed to add anti-aliasing in the Automation Editor with some minor points that need addressing:

  • The Shading is inconsistent (Although it was inconsistent in some themes before)

  • I'm not using the is_selected Boolean (Can't seem to find it's use)

  • I don't catch the points if they aren't in the "Visible area" as was done before

This is the result thus far:
before
After:
after

Keep in mind this is at extreme zoom in

@tresf
Copy link
Member

@tresf tresf commented Feb 27, 2017

@TheTravelingSpaceman @Umcaruje that shading won't ever (EVER) line up unless we either set a fixed gradient (yuck).

Can we just abolish it? I see this is being done against the old theme, has the new theme gotten rid of the gradient or is it still there?

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 27, 2017

Can we just abolish it?

Are you talking about the gradient?

The new theme works just fine since it doesn't use a gradient.
image

@tresf
Copy link
Member

@tresf tresf commented Feb 27, 2017

The new theme works just fine

👍

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Feb 27, 2017

The new theme works just fine since it doesn't use a gradient.

Actually it still does use one, but I agree this can be removed really. @RebeccaDeField thoughts?

drawLevelTick( p, it.key() + i, values[i],
is_selected );
float nextValue;
if ( m_pattern->valuesAfter( ( it + 1 ).key() ) != NULL )

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

You should have brackets for the if/else statement, and there should be no space between if and the parenthesis: if(...

This comment has been minimized.

@WalterSmuts

WalterSmuts Feb 27, 2017
Author Contributor

I thought it looked neater without the brackets but I'll add them to make it consistent?

This comment has been minimized.

@Umcaruje

Umcaruje Mar 8, 2017
Member

You're still missing {} brackets here

path.lineTo( QPointF( xCoordOfTick( ( it + 1 ).key() ), yCoordOfLevel( nextValue ) ) );
path.lineTo( QPointF( xCoordOfTick( ( it + 1 ).key() ), yCoordOfLevel( 0 ) ) );
path.lineTo( QPointF( xCoordOfTick( it.key() ), yCoordOfLevel( 0 ) ) );
p.fillPath( path, graphColor() );

This comment has been minimized.

@Umcaruje

Umcaruje Feb 27, 2017
Member

The is_selected variable was used when a particular automation point was selected, but this is disabled and broken atm, it would requre some CSS changes to be properly implemented, and we can save that for when that feature gets fixed.

This comment has been minimized.

@WalterSmuts

WalterSmuts Feb 27, 2017
Author Contributor

So can I delete the code referencing the is_selected variable and related code?

This comment has been minimized.

@Umcaruje

Umcaruje Mar 8, 2017
Member

It's good when it's commented out as now.

@RebeccaDeField
Copy link
Contributor

@RebeccaDeField RebeccaDeField commented Feb 27, 2017

Actually it still does use one, but I agree this can be removed really. @RebeccaDeField thoughts?

The gradient is just for a touch of depth and it can definitely be flattened. I do think it's important to keep some transparency so that the grid can show through for usability. I'm thinking we could give the whole automation graph the same transparency as the top stop.

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 27, 2017

I'm thinking we could give the whole automation pattern the same transparency as the top stop.

What's the top stop?

And is it a problem that the Travis-CI failed it's tests?

@tresf
Copy link
Member

@tresf tresf commented Feb 28, 2017

What's the top stop?

I believe @RebeccaDeField is recommending to use the top-most automation point as the gradient end point for the entire section so that they draw without segmentation.

@RebeccaDeField
Copy link
Contributor

@RebeccaDeField RebeccaDeField commented Feb 28, 2017

@TheTravelingSpaceman It is currently using a gradient with two stops (top and bottom). If we wanted to take away the gradient, we would only have one shade of transparency. I'm suggesting that we use the transparency of stop:0 which seems to be 180.

Sorry if that was confusing 😅

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 28, 2017

@RebeccaDeField So do we change that in the CSS or the c++ code? I can't seem to find the right place to limit it in the c++ code :/

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Feb 28, 2017

@TheTravelingSpaceman I think it would be wiser to actually comment out the code rather than remove it, cause we want that feature back sometime.

@WalterSmuts WalterSmuts force-pushed the WalterSmuts:Anti-Aliasing branch from 0031766 to 10de750 Feb 28, 2017
@WalterSmuts WalterSmuts changed the title Aliasing in AutomationPatternView; ERROR: Doesn't draw unreferencing tracks Aliasing in AutomationPatternView and AutomationEditor fixed Feb 28, 2017
@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Feb 28, 2017

So? What's the next step?

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Mar 2, 2017

@RebeccaDeField
Copy link
Contributor

@RebeccaDeField RebeccaDeField commented Mar 4, 2017

@TheTravelingSpaceman Sorry for the late reply, this somehow got buried in my inbox. You will need to remove the gradient in the style.css file if that is what you're asking. 😊

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Mar 5, 2017

@Umcaruje Should be fine to merge now

@WalterSmuts WalterSmuts force-pushed the WalterSmuts:Anti-Aliasing branch from a57b823 to 7d0355e Mar 10, 2017
WalterSmuts added 2 commits Mar 11, 2017
@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Mar 12, 2017

@Umcaruje I've looked at the two problems you've indicated and can't find a proper way to fix it (That I understand and am confident that it won't break something elsewhere).

I did "fix" the bottom pixel error in the final commit but if feels a bit hackish. Maybe the person who wrote the code originally should take a look (@jmuzz)

The problems however has nothing to do with the things I've added and can therefore be fixed separately in another PR. I would like to fix it but unfortunately I'm starting school tomorrow.

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Mar 12, 2017

Did another test. It looks really smooth, but you're off by a pixel when drawing in the automation editor:
...
As you see, there's a pixel gap on the bottom.

But this gap is there already in 1.0.3

I did "fix" the bottom pixel error in the final commit but if feels a bit hackish.

Works, but maybe not if fixing the top margin error too.

Also there seems to be some overflowing on the top end of the automation editor, this was probably not introduced with this PR, but it's just something that I've noticed now.

https://github.com/LMMS/lmms/blob/master/include/AutomationEditor.h#L176
If we don't take the bottom gap into account, Changing TOP_MARGIN value to ~20 seem to take care of this. I think rather the theming folks needs to take a look at this as it's a separate issue.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Mar 12, 2017

But this gap is there already in 1.0.3

Huh, I never noticed that. Guess I never looked hard enough. Anyways @TheTravelingSpaceman the last commit doesn't fix it completely. I suggest you revert it, we merge this and open a seperate issue for the top margin and the pixel gap.

This reverts commit ff80186.
@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Mar 12, 2017

Reverted as requested. Should be fine to merge now @Umcaruje

@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Mar 12, 2017

Reverted as requested. Should be fine to merge now @Umcaruje

I'm pretty sure he meant reverting 940c766

@WalterSmuts
Copy link
Contributor Author

@WalterSmuts WalterSmuts commented Mar 12, 2017

@zonkmachine I thought that was what I was doing. Was under the impression that the command reverts to the state that the commit was at that point. So just making sure now... I need to execute the following code to have it right:
git revert 4e127a7
git revert 940c766

The first line reverts the incorrect "revert" that I did. And the second is what I should have done initially

WalterSmuts added 2 commits Mar 12, 2017
This reverts commit 940c766.
@zonkmachine
Copy link
Member

@zonkmachine zonkmachine commented Mar 12, 2017

git revert 4e127a7
git revert 940c766

Yes, this works.

For the future. I usually do rebase -i and remove the lines of the commits I want removed as it's neater ( first back up! ) .

@tresf
Copy link
Member

@tresf tresf commented Mar 12, 2017

revert reverts a single commit. reset --hard rewinds back in time to a particular commit.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Mar 13, 2017

👍

@Umcaruje Umcaruje merged commit 899e386 into LMMS:master Mar 13, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@RebeccaDeField
Copy link
Contributor

@RebeccaDeField RebeccaDeField commented Mar 13, 2017

Nice work @TheTravelingSpaceman! 😁

@WalterSmuts WalterSmuts deleted the WalterSmuts:Anti-Aliasing branch Mar 13, 2017
@Umcaruje Umcaruje mentioned this pull request Mar 14, 2017
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.