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

Stricter checks for FlxG.bitmap.maxTextureSize #3279

Merged
merged 5 commits into from
Nov 4, 2024

Conversation

ACrazyTown
Copy link
Contributor

Checks if any of the Lime OpenGL defines are defined & also checks if FlxG.stage.window is hardware accelerated

@Geokureli
Copy link
Member

Can we add a new define to FlxDefines.HelperDefines, instead? like FLX_HAS_MAX_TEXTURE_SIZE or something?

@ACrazyTown
Copy link
Contributor Author

Can we add a new define to FlxDefines.HelperDefines, instead? like FLX_HAS_MAX_TEXTURE_SIZE or something?

I was thinking about that too, would make things a lot cleaner to manage.

Also, would it be better to use Null<Int> instead of returning -1 when it's not possible to get the max texture size?

@Geokureli
Copy link
Member

Also, would it be better to use Null<Int> instead of returning -1 when it's not possible to get the max texture size?

I know -1 isn't a great solution, but I rarely think Null is much better, compared to say, enums. either way, lets stick with -1 rather than introducing a potentially breaking change

I'll test this PR soon

@ACrazyTown
Copy link
Contributor Author

ACrazyTown commented Oct 31, 2024

I know -1 isn't a great solution, but I rarely think Null is much better, compared to say, enums. either way, lets stick with -1 rather than introducing a potentially breaking change

Is it a breaking change if right now trying to access FlxG.bitmap.maxTextureSize on software rendered/non-OpenGL builds will result in a crash because the GL context isn't available?

@ACrazyTown
Copy link
Contributor Author

Also, I was thinking of adding a check in FlxGraphic to warn users if they were making graphics that were larger than maxTextureSize, could I sneak that in here or should that be a seperate PR?

@Geokureli
Copy link
Member

go for it

if (max != -1)
{
if (width > max || height > max)
FlxG.log.warn('Graphic dimensions (${width}x${height}) exceed the maximum allowed size (${max}x${max}), which may cause rendering issues.');
Copy link
Member

Choose a reason for hiding this comment

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

What kind of rendering issues will happen? I'm debating whether this should be an error or not

Copy link
Contributor Author

@ACrazyTown ACrazyTown Nov 1, 2024

Choose a reason for hiding this comment

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

From my understanding OpenGL won't upload the texture to the GPU at all and any attempts to render the texture will be replaced with a fallback texture (usually just black or whatever), probably should be an error yeah

@ACrazyTown
Copy link
Contributor Author

Idk why CI failed

@Geokureli
Copy link
Member

The CI failure seems unrelated, I usually just rerun them

@Geokureli
Copy link
Member

Geokureli commented Nov 3, 2024

The remaining CI failures DO seem related. I'll have to try unit tests locally. For now I'll rerun the jobs, again, just in case it's nothing

@Geokureli Geokureli added this to the 5.9.0 milestone Nov 3, 2024
@@ -254,7 +254,9 @@ class FlxDefines
define(FLX_TRACK_GRAPHICS);

#if (lime_opengl || lime_opengles || lime_webgl)
define(FLX_OPENGL_AVAILABLE);
// FlxG.stage.window.context.attributes.hardware is not always defined during unit tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be better to check if FlxG.stage.window.context.attributes.hardware (or whatever part of it that fails) is defined rather than outright skipping it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so. If it's not defined in any other context I want it to fail loudly, and I don't think we can really unit test gl stuff right now

@Geokureli Geokureli merged commit b39f68d into HaxeFlixel:dev Nov 4, 2024
11 checks passed
@Geokureli
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants