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

Implement 2d filter resolution scaling. #903

Closed
wants to merge 5 commits into from

Conversation

Yanrishatum
Copy link
Contributor

Resolves #885

Changes overview

  • Object.clipBounds now expects scaleX and scaleY arguments in order to properly calculate boundaries mask with respect to filter resolution scaling.
  • Moved if (maxExtent >= 0) check inside autoBounds, because there's no point in having it ouside, since it would never execute if autoBounds is disabled.
  • Added RenderContext.tmpPoint alongisde tmpBounds - used for when autoBounds is disabled to transfer scaling.
  • AbstractMask got new internal variables in order to keep consistent with resolution scaling. Also exactly because of AbstractMask using double-filter approach - resolution variables are non-inline properties.
  • Added Filter.resolutionScale that will affect the rendering resolution.
  • Added Filter.useScreenResolution that will use scene scale mode to map filter texture to screen pixels. Fixes long-standing issue of scaleMode downscaling filter textures for PA games. Is affected by resolutionScale.
  • Filter.getBounds not receives scale:Point, which will contain the resolution scaling dictated by by resolutionScale/useScreenResolution. Can be modified in order to alter the resulting resolution scale.

Unresolved issues

  • Resolution scaling does not work properly if filter is attached to a Scene.
    Easy solution is to just ignore it and add a note that resolution scaling is not supported for Scene. But I'd rather fix the issue.
    Another solution is to override clipBounds entirely and use slightly different math in clipping logic, in order to avoid incorrect clipping. Mainly publishing this PR currently so I can work on docs instead of keeping those changes in git stash.

@ncannasse
Copy link
Member

What is the status of this PR ? ;)

@Yanrishatum
Copy link
Contributor Author

Yanrishatum commented Nov 22, 2020

Same as it was at a time of posting: Resolution scaling still does not properly work when attached to the Scene. If you want, I can get to fixing it faster. ;) Not like there no other PRs that are pending.

@Yanrishatum Yanrishatum marked this pull request as ready for review December 18, 2020 11:38
@Yanrishatum
Copy link
Contributor Author

@ncannasse Ready for review btw.

@Yanrishatum
Copy link
Contributor Author

Thanks to @eoy, discovered a number of issues with current implementation. Working on a fix.

Fix tile size issues on final tile rendering.
Revert accidental console change.
@Yanrishatum
Copy link
Contributor Author

Issues are ironed out. Waiting for review. @ncannasse

Used to calculate filter rendering resolution.
**/
@:dox(hide)
public var tmpPoint = new h2d.col.Point();
Copy link
Member

Choose a reason for hiding this comment

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

please make it private without documentation and access it with @:privateAccess, it's not meant to be public but implementation detail. Alternatively, you can use a static var in Object.

@@ -7,6 +7,9 @@ class Hide extends Filter {
public var input : h2d.Tile;
public var maskVisible : Bool;

public var inputWidth:Int;
public var inputHeight:Int;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to make them public ?

Copy link
Member

Choose a reason for hiding this comment

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

Used by AbstractMask

@ncannasse
Copy link
Member

I haven't double checked all the scaling calculus but I guess you tested it well ;)

@trethaller
Copy link
Member

Using useScreenResolution breaks masks when scene is scaled, apparently this calculation in AbstractMask:

	// move from tex a to tex b
	maskMatrix.x /= tile.width;
	maskMatrix.y /= tile.height;

is missing the scale factor.

@trethaller trethaller reopened this Oct 29, 2021
Sunjammer pushed a commit to Sunjammer/heaps that referenced this pull request Nov 4, 2021
Squashed commit of the following:

commit 68215af
Author: trethaller <tom.rethaller@gmail.com>
Date:   Wed Oct 27 15:26:52 2021 +0200

    remove tmpPoint from RenderContext API

commit f2eef31
Author: trethaller <tom.rethaller@gmail.com>
Date:   Wed Oct 27 15:25:46 2021 +0200

    add Filter.defaultUseScreenResolution

commit fdc50cd
Merge: 47d3854 4b5dbf0
Author: trethaller <tom.rethaller@gmail.com>
Date:   Wed Oct 27 15:02:52 2021 +0200

    Merge remote-tracking branch 'remotes/origin/master' into rfc_885

commit 47d3854
Author: Yanrishatum <krabanek@gmail.com>
Date:   Sun Dec 27 11:42:30 2020 +0300

    Fix positioning of filters that change dx/dy.
    Revert accidental console change.

commit 13be38c
Author: Yanrishatum <krabanek@gmail.com>
Date:   Mon Dec 21 00:37:52 2020 +0300

    Fix overclipping on scaled filters
    Fix tile size issues on final tile rendering.

commit 0769ff9
Merge: 0690ae6 f210705
Author: Yanrishatum <krabanek@gmail.com>
Date:   Fri Dec 18 14:38:16 2020 +0300

    Merge branch 'master' into rfc_885

commit 0690ae6
Author: Yanrishatum <krabanek@gmail.com>
Date:   Fri Dec 18 14:36:42 2020 +0300

    Fix Scene filters with resolution scaling

commit b0611b1
Author: Yanrishatum <krabanek@gmail.com>
Date:   Mon Nov 2 17:31:22 2020 +0300

    Implement 2d filter resolution scaling.
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.

[RFC] Resolution scaling in filters
3 participants