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

(ATTR|GLOBAL)_shadow_blend doesn't use material.xplane.blendRatio #426

Closed
tngreene opened this issue Apr 22, 2019 · 7 comments
Closed

(ATTR|GLOBAL)_shadow_blend doesn't use material.xplane.blendRatio #426

tngreene opened this issue Apr 22, 2019 · 7 comments

Comments

@tngreene
Copy link
Contributor

Correctness bug: _shadow_blend doesn't let the user set .blendRatio for shadow blend. It simply exports
ATTR_shadow_blend
with nothing after it.

X-Plane has been using the default value .5 this whole time and I guess nobody noticed or cared enough to file a bug.

@tngreene tngreene self-assigned this Apr 22, 2019
@tngreene tngreene added Bug New Unit Test priority normal good first issue Bugs in one or two easy plces that can be fixed quickly help wanted labels Apr 22, 2019
@tngreene
Copy link
Contributor Author

This would also be a good moment to make a blend mode unit test.

@kbrandwijk
Copy link
Contributor

Looking into this, .blendRatio is specifically defined as 'Alpha cutoff ratio'. I'm not sure, based on the name and description of that property, if blendRatio should be used, or a separate shadowBlendRatio should be introduced?

@kbrandwijk
Copy link
Contributor

kbrandwijk commented Aug 18, 2019

Looking further, this decision would also impact the converter (see #455)

@tngreene
Copy link
Contributor Author

I think we should reuse blendRatio instead of making a new property (we are extremely new property averse since they are very hard to cleanly remove).

  • The ratio in ATTR_no_blend <ratio> and ATTR_shadow_blend <ratio> represent the same range of values (0.0-1.0)`
  • Artists are very unlikely to switch Blend Mode often and then re-assign blendRatio
  • If I'm wrong and they do switch Blend Modes a lot, we can add this in later, and use the updater to copy to "Blend Mode is Shadow Blend + blendRatio" -> shadowBlendRatio

Another change, capitalize C and R in Alpha Cutoff Ratio. The name is good but we'll need to change the description based on Shadow Blend vs Alpha Cutoff.

@kbrandwijk
Copy link
Contributor

I agree that they are so close together in functionality and meaning, and the blend modes being mutually exclusive of course, so reusing the prop makes sense indeed.

However, I could find a way to change the text that is shown in the textbox (that's just a parameter when you add it to the panel), but I couldn't figure out how to dynamically change the description (shown in the tooltip).

@tngreene
Copy link
Contributor Author

I could have sworn you could have changed the tooltip. I guess not! Strange.

I care more about not changing the data model than not changing the UI.

the tool tip can be something general about how a Blend Ratio controls the output and we can have different labels beneath with the description we want. Will decide later, but I'm welcome to suggestions

@tngreene
Copy link
Contributor Author

And, very small reminder to myself in the future, there's a TODO comment in xplane_249_material_converter that is set to be removed after this is finished.

@tngreene tngreene removed the good first issue Bugs in one or two easy plces that can be fixed quickly label Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants