NobleEngine icon indicating copy to clipboard operation
NobleEngine copied to clipboard

Graphics.sprite.setAlwaysRedraw to false by default

Open simjnd opened this issue 3 years ago • 3 comments

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.

simjnd avatar Sep 02 '22 22:09 simjnd

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?

Mark-LaCroix avatar Sep 05 '22 21:09 Mark-LaCroix

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.

simjnd avatar Sep 06 '22 06:09 simjnd

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 avatar Sep 06 '22 19:09 Mark-LaCroix

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 avatar Mar 30 '23 05:03 Mark-LaCroix

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

Mark-LaCroix avatar Mar 30 '23 05:03 Mark-LaCroix