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

Graphics.sprite.setAlwaysRedraw to false by default #36

Merged
merged 3 commits into from
Mar 30, 2023

Conversation

simjnd
Copy link
Contributor

@simjnd simjnd commented Sep 2, 2022

In Noble.lua at line 91 you set Graphics.sprite.setAlwaysRedraw(true) when the SDK Documentation specifies that it can be faster only in very specific circumstances (namely when you have a ton of sprites moving at once.

I am not sure if there were any reasons why you opted-in, but I think it would be wiser to have this set to false by default, so that all the simpler projects that rely on NobleEngine can benefit from the battery and performance improvements of fewer draw calls, and only when they reach this specific bottleneck can they opt-in to this behaviour, as it is with the default SDK.

@Mark-LaCroix
Copy link
Member

I've been struggling with this one. Using this makes things a lot simpler for the user, especially beginners who don't understand draw calls or don't want to manually manage them, but as you point out, it comes at a notable performance cost.

I think you're probably right, but I'm going to have to think a bit more about it before I make this change. Mainly, I want to make sure that the docs better explain the feature. Perhaps I could surface it as an argument in Noble.new() with the default being false? What do you think?

@simjnd
Copy link
Contributor Author

simjnd commented Sep 6, 2022

It's definitely a good idea to offer a good explanation of the feature in the docs. Since sprites automatically handle the dirty-tracking and optimize for the least amount of draw calls necessary, the explanation should probably push users into using sprites for most things they want to render this way we can avoid them having to worry about this.

Instead of putting in Noble.new() how would you feel about maybe creating a Noble.config() method that can take a table with keys to configure certains parameters?

Noble.config({
  setAlwaysRedraw = true,
  showFPS = true
})

Noble.new(...)

This could serve for future more advanced configuration, for instance if you want to configure the threshold used to determine whether a button is held or not (say instead of 3 frames you want a threshold of 5 frames), or other things I can't think of at the moment.

@Mark-LaCroix
Copy link
Member

Yeah, I like that solution. I think the way I'd do it is letting users pass a config table object as an optional argument in Noble.new, and adding Noble.config as a method that can be run at any time (because sometimes you want to change things at runtime).

@Mark-LaCroix Mark-LaCroix merged commit 875d796 into NobleRobot:main Mar 30, 2023
github-actions bot pushed a commit that referenced this pull request Mar 30, 2023
Graphics.sprite.setAlwaysRedraw to false by default
@Mark-LaCroix
Copy link
Member

Mark-LaCroix commented Mar 30, 2023

I've gone in and updated Noble.lua to add a configuration API, which allows user to set these values:

{
	defaultTransitionDuration = 1,
	defaultTransitionHoldDuration = 0.2,
	defaultTransitionType = Noble.TransitionType.DIP_TO_BLACK,
	enableDebugBonkChecking = false,
	alwaysRedraw = true,
}

I did leave alwaysRedraw as true for now, just to avoid any breaking changes (although this does introduce a smaller one), but it should give users control over it at least. I suspect this will get further updates in the future.

@Mark-LaCroix
Copy link
Member

I'm very happy that your PR inspired this change, btw!

@Mark-LaCroix Mark-LaCroix mentioned this pull request Apr 8, 2023
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