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

Added support for shader based scaling on Next #1826

Closed
wants to merge 7 commits into from

Conversation

Seanw265
Copy link

No description provided.

@Gama11
Copy link
Member

Gama11 commented Apr 22, 2016

Does this still work with OpenFL legacy?

I guess the old pull request can be closed?

@Gama11 Gama11 added this to the 4.2.0 milestone Apr 22, 2016
@@ -120,8 +120,8 @@ class FlxPointer
*/
public inline function setGlobalScreenPositionUnsafe(newX:Float, newY:Float):Void
{
_globalScreenX = Std.int(newX / FlxG.scaleMode.scale.x);
_globalScreenY = Std.int(newY / FlxG.scaleMode.scale.y);
_globalScreenX = Std.int(FlxG.scaleMode.mouseMultiplier.x * newX / FlxG.scaleMode.scale.x);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is in FlxPointer, it affects touches as well. Because of that, pointerMultiplier is probably a better name than mouseMultiplier.

@Seanw265
Copy link
Author

It doesn't actually scale on legacy but it doesn't crash either. It just runs as BaseScaleMode.

I suppose its possible to reimplement it the old way with a compiler conditional. I'll look into that right now.

And yes the other one can be closed.

@larsiusprime
Copy link
Member

Looks cool :) What's the use code, same as the other one?

@Seanw265
Copy link
Author

@larsiusprime Yup same as the other one!

@Gama11
Copy link
Member

Gama11 commented Apr 22, 2016

You don't have to update each file individually using GitHub's web UI, that seems very cumbersome. You can use a git client like SourceTree to push to your fork's branch.

@Gama11
Copy link
Member

Gama11 commented Apr 22, 2016

Btw, we have automated code style checks, yours doesn't match Flixel's in a few places:

https://codeclimate.com/github/HaxeFlixel/flixel/pull/1826

Feel free to ignore the Invalid method name signature ones as false positives.

}

override public function clone():BitmapFilter {
return super.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method can probably be removed, considering it just calls super?

@MSGhero
Copy link
Member

MSGhero commented Apr 22, 2016

SourceTree is confusing af to start with, but it's super useful once you do. Alternatively, github also has a desktop client

@Seanw265
Copy link
Author

So I have a bunch of changes that improve on the classes I made that I'll push as soon as I'm back at the computer. It adds support for legacy and cleans up a lot of the styling issues.
I took care of the override of __preparePass
There might be a way to take care of overriding __growBounds but changing that back to the original breaks the shaders' math. That math is very convoluted and complicated and I'm tempted to say we should leave it as is for now.

@Seanw265
Copy link
Author

Ok I just pushed another commit that gets rid of all the internal openfl method overrides. It also gets rid of really weird funky math that was necessary in each shader in order to position the screen correctly.

@Gama11
Copy link
Member

Gama11 commented May 13, 2016

So, is this on hold because of openfl/openfl#1128?

@MSGhero
Copy link
Member

MSGhero commented May 13, 2016

That issue was so recent and this PR has been sitting a while

@Gama11
Copy link
Member

Gama11 commented May 13, 2016

I don't see how that answers my question? :P

@Seanw265
Copy link
Author

Hang on I may have found a small bug. I'll run some tests tonight and get back to you guys soon.

@MSGhero
Copy link
Member

MSGhero commented May 13, 2016

Oh I thought you were asking "Has this been on hold this whole time bc of this issue?"

Is it the same result if openfl implements it, given that this is acting on a single DisplayObject (FlxGame) and openfl's would act on the visible stage/screen buffer?

@Gama11
Copy link
Member

Gama11 commented May 13, 2016

Ah, I should have said "is this now on hold".

Well, there usually isn't too much going on in a flixel game that isn't part of FlxGame.

@larsiusprime
Copy link
Member

No, I don't think this should be on hold; it's not clear whether OpenFL will agree it should be part of the core API (although I think it should) and this is working splendidly in DQ.

Just noticed CI / codeclimate are failing, guess we can fix those up.

@Tiago-Ling
Copy link
Contributor

Any news on this?

@JoeCreates
Copy link
Member

@Tiago-Ling What is the android+next game scaling issue you mentioned? Is there another issue open for that?

@Tiago-Ling
Copy link
Contributor

@JoeCreates Actually no issues opened, but it may be the situation for one - not directly related to the scaling though.

The scaling problem is this:

  • Scaling is done with nearest neighbor algorithm, which can really mess up pixel art when using non-integer factors. This is expected and can be countered to an extent through scaling thresholds and letter boxing.
  • The real problem is that a lot of devices use soft buttons for their home, back and menu system buttons. Since there's no way (i think) to really hide them this leads to some really unconventional stage sizes which will result in a sub-100% zoom and will visibly affect non-antialiased graphics (especially text).

The idea would be to use this scaling with shaders to alleviate this problem. One way to try this is to scale the game first using nearest neighbor to the desired integer (2x, 3x, etc) then apply the decimal part using a bilinear filter or something better than nn. This is what Shovel Knight does, according to openfl/openfl#1128

On the other hand, the problem which might need an issue is that i couldn't get ScaleMode to work correctly in Android (probably iOS too) when using next. I had to manually scale the game here to achieve what works perfectly in legacy.

@JoeCreates
Copy link
Member

Wasnt the first scaling issue always a problem even before next? I actually
prefer nearest neighbour for pixel art games, but I certainly see that its
a problem for higher res stuff.

On 27 Jul 2016 12:27, "Tiago Ling Alexandre" notifications@github.com
wrote:

@JoeCreates https://github.com/JoeCreates Actually no issues opened,
but it may be the situation for one - not directly related to the scaling
though.

The scaling problem is this:

  • Scaling is done with nearest neighbor algorithm, which can really
    mess up pixel art when using non-integer factors. This is expected and can
    be countered to an extent through scaling thresholds and letter boxing.
  • The real problem is that a lot of devices use soft buttons for their
    home, back and menu system buttons. Since there's no way (i think) to
    really hide them this leads to some really unconventional stage sizes which
    will result in a sub-100% zoom and will visibly affect non-antialiased
    graphics (especially text).

The idea would be to use this scaling with shaders to alleviate this
problem. One way to try this is to scale the game first using nearest
neighbor to the desired integer (2x, 3x, etc) then apply the decimal part
using a bilinear filter or something better than nn. This is what Shovel
Knight does, according to openfl/openfl#1128
openfl/openfl#1128

On the other hand, the problem which might need an issue is that i
couldn't get ScaleMode to work correctly in Android (probably iOS too)
when using next. I had to manually scale the game here to achieve what
works perfectly in legacy.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1826 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACbEvHi2DGjM4S60l-Ddra0799bxDEanks5qZ0CVgaJpZM4IN2Bu
.

@Tiago-Ling
Copy link
Contributor

In legacy the game scaling works fine here - in next i've experienced (i'll create an issue later if there isn't one already):

  • Game centered on the top-left part of the screen, with only the bottom-right part inside the screen
  • Game scaled to 1/4 of the screen

Nearest neighbor causes problems for non-integer scales - try scaling a pixel-art game to 1.15, 1.25 to see what i mean: some hard edges will be of different sizes and things will be uneven. This is a bigger problem for pixel-perfect fonts which will get terribly jagged and lose a lot of readability.

@JoeCreates
Copy link
Member

I know, but if you scale more, say 3 or more times, those extra lines
become negligable. You dont tend to scale a pixel art game by 1.5 times. I
would prefer the extra pixels to be a color in the existing palette rather
than have bilinear create new colors.

What I meant regarding lagacy was that legacy always used nearest
neighbour, too, so that issue isn't new.

On 27 Jul 2016 14:57, "Tiago Ling Alexandre" notifications@github.com
wrote:

In legacy the game scaling works fine here - in next i've experienced
(i'll create an issue later if there isn't one already):

  • Game centered on the top-left part of the screen, with only the
    bottom-right part inside the screen
  • Game scaled to 1/4 of the screen

Nearest neighbor causes problems for non-integer scales - try scaling a
pixel-art game to 1.15, 1.25 to see what i mean: some hard edges will be of
different sizes and things will be uneven. This is a bigger problem for
pixel-perfect fonts which will get terribly jagged and lose a lot of
readability.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1826 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACbEvBm6BLAdogbgHnSuy7Gk6nhmm3bMks5qZ2PDgaJpZM4IN2Bu
.

{
super();

if (Std.is(shader, Shader))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this unnecessary since we're leveraging Haxe's type system to ensure people send valid Shader types?

postProcess.setUniform("scaleY", scaleY);
this.scaleX = scaleX;
this.scaleY = scaleY;
this.pointerMultiplier = new flixel.math.FlxPoint(1 / scaleX, 1 / scaleY);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary memory allocation, just do: this.pointerMultiplier.set(1 / scaleX, 1 / scaleY);

@Gama11 Gama11 removed this from the 4.3.0 milestone May 21, 2017
@Gama11
Copy link
Member

Gama11 commented Feb 4, 2019

This is a bit outdated by now and would have to be updated to support OpenFL 8.

@Gama11 Gama11 closed this Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants