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

Mark base2d texture as shared #912

Closed
wants to merge 2 commits into from

Conversation

Yanrishatum
Copy link
Contributor

Reasoning: Since I discovered the @shared qualifier, I seriously wondered why base2d does not expose its texture, since it would allow simplification of shaders by omitting double-passing of same texture during rendering of 2d elements that performs multiple texture samples. And with introduction of .size() support, I decided to mark it as shared.

Here's an example of shader that uses both new .size() feature and shared texture of Base2d to produce subpixel antialiasing (useful for PA games, and requires setting smooth = true):

package h3d.shader;

class PixelartAA extends hxsl.Shader {
	
	static var SRC = {
		
		var texture : Sampler2D;
		@var var calculatedUV:Vec2;

		function subpixelAA(uv:Vec2):Vec2 {
			var size = texture.size();
			var deriv = fwidth(uv) * .5;
			var uvmin = (uv - deriv) * size; // top-left edge of the screen pixel
			var uvmax = (uv + deriv) * size; // bottom-right edge of the screen pixel
			var fmax = floor(uvmax);
			var t = (fmax - uvmin) / (uvmax - uvmin) * (fmax - floor(uvmin));
			uv = ((fmax + 0.5 - t) / size);
		}

		function __init__fragment() {
			calculatedUV = subpixelAA(calculatedUV);
		}

	}

}

Also question: Should I PR that shader in, or it's too specific of a shader for stock Heaps? :)

@Yanrishatum
Copy link
Contributor Author

Marked color as shared as well with same justification. Use-case is fairly clear: By being able to access object tint value, it's possible to add tinting processing after pixelColor was calculated. In my case, I needed to bypass pixelColor altogether in order to make SDF outline work properly (i.e. apply tint to internal color but only tint outline faintly)

@ncannasse
Copy link
Member

I don't think that's good this way. First the names "texture" and "color" are way too generic to be shared.

Also, it's a bit against the philosophy of the framework to assume that your shader will have a texture and a color material. The idea is clearly instead to have a chain of transforms on a single pixelColor, without having to know about previous/next steps.

Being able to access the texture directly might work for some shaders but it won't in the general case since you might have other shaders "in between" doing other color transforms that will not be performed by the second fetch. So I think it's limited to some specific shaders that can only apply as "initial color transform" and for these I think passing the texture and color manually is good enough.

@ncannasse ncannasse closed this Nov 22, 2020
@Yanrishatum
Copy link
Contributor Author

I understand where this is coming from, but I do not agree that they should not be shared. Access to Base2d texture simplifies a lot, and since you need to explicitly declare sampler without qualifiers to access it (I.e. var texture:Sampler2D), I don't think that generic name is a problem, since @param var texture:Sampler2D is treated as unique and would not use the shared one.
And I absolutely do not agree that manual color/texture pass is "good enough". Take SpriteBatch that uses multiple textures for example. Manually added shader would have no idea which texture is currently attached during the drawcall, because it's automatically managed by BatchState.
As for color, it's one of the Drawable elements, and being able to know what color tint was used without doing unnecessary legwork is a lot more robust, but generic name does pose a problem in this case, and I'm fine skipping it. But not for texture.

Alternatively, I'd propose a new qualifier that lets you forcefully borrow a param from another shader. Since it has to be explicitly used - there won't be naming conflicts of shared params, and will also allow to get necessary information if user knows what he needs, and it's on his shoulders to not break the shader pipeline. I.e. something like that:

// Should have same type/name and point at the source shader it borrow param from.
@borrow(h3d.shader.Base2d) var texture:Sampler2D;
// Alternatively (but more complex) - declare param name in meta and let user use whatever name he wants.
@borrow(h3d.shader.Base2d, texture) var sourceTex:Sampler2D;

HXSL is already inaccessible to most people as it is, adding yet another qualifier people has no idea about won't change much.

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.

2 participants