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

Add FlxBuffer to hide Vector<Float> as a Array<T> #3192

Open
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

Geokureli
Copy link
Member

@Geokureli Geokureli commented Jun 21, 2024

The Problem

Many rendering systems are littered with Array<Float>s meant to be used like so:

rects.push(rect.x);
rects.push(rect.y);
rects.push(rect.width);
rects.push(rect.height);

transforms.push(matrix.a);
transforms.push(matrix.b);
transforms.push(matrix.c);
transforms.push(matrix.d);
transforms.push(matrix.tx);
transforms.push(matrix.ty);

and

for (i in 0...list.length)
{
	final pos = j * 3;
	final charCode = Std.int(list[pos]);
	final x = list[pos + 1];
	final y = list[pos + 2];
	doSomething(charCode, x, y);
}

This can be both hard to read and susceptible to human error compared to using Array<FlxRect>, but not only does the current way greatly reduce objects that need to be garbage collected, many lime/open features specifically require Vector<Float> or Array<Float>, anyway

The Solution

FlxBuffer and FlxBufferArray, under the hood they actually an Array<Float> or Vector<Float>, but when writing code with them, you can easily deal with them as though they were an Array<{x:Float, y:Float}>. Truly the best of both worlds!

The above code, while compiling to nearly identical source code, will look like this:

rects.push(rect);
transforms.push(matrix);

and

for (item in list)
	doSomething(item.charCode, item.x, item.y);

To Do

  • Manually verify the compiled js/cpp never instantiates anonymous structs in any of the utilizing code
  • Do benchmark tests, comparing the old code vs the new
    • Html5
    • hxcpp
    • hl
    • neko
    • flash
  • Compare general speed of openfl.Vector vs Array
    • Html5
    • hxcpp
    • hl
    • neko
    • flash
  • See if there's a way to automatically detect if changes cause items to be instantiated (might not be possible)

@MaybeMaru
Copy link
Contributor

Pretty cool to see updates on this! Just make sure to not use typedefs structures stuff for the final thing since those are pretty slow compiling to dynamic

@Geokureli
Copy link
Member Author

Geokureli commented Jun 22, 2024

Pretty cool to see updates on this! Just make sure to not use typedefs structures stuff for the final thing since those are pretty slow compiling to dynamic

I don't know what you mean by "pretty slow compiling to dynamic", typedefs compile crazy fast to non-static targets. the goal here is to have seemingly typed arrays/vectors without incurring any garbage collection from instances of the types

I've done some initial tests and no types are actually used as everything is inlined away, see here: https://try.haxe.org/#ADa541d7
the build macro adds 0.091s of compile time, which i think it worth the added readability

But I plan to do actual benchmark tests with these flixel classes, too

@MaybeMaru
Copy link
Contributor

In hxcpp (im not sure about other targets) typedefs compile has annonymous dynamic structures but yeah better to double check with benchmarks.

@Geokureli
Copy link
Member Author

Geokureli commented Jun 22, 2024

In hxcpp (im not sure about other targets) typedefs compile has annonymous dynamic structures but yeah better to double check with benchmarks.

Again I don't see why we're talking about compiling here, hxcpp takes me almost an hour to build from scratch, already and dynamic targets take an extra 10th of a second

The benchmarks I'm referring to are for runtime performance because I don't see any concern for compile time. It sound like maybe you're actually referring to runtime performance but for some reason keep using the word "compile"

@Geokureli
Copy link
Member Author

@crowplexus and @NovaTheFurryDev, you voted thumbs down, care to share your concerns?

@crowplexus
Copy link

removing the DV, my biggest concern with this was perhaps it being slower than the current method, I will definitely give this a try later on a blank project to see if it makes a major difference in comparison to the old method

@Geokureli
Copy link
Member Author

removing the DV

What's the DV?

@crowplexus
Copy link

removing the DV

What's the DV?

downvote or thumbs down

case TInst(local, [type]):
return buildBuffer(getFields(type, includeGetters), type, isVector);
default:
throw "Expected TInst";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think using Context.error() would be better than throwing since it shows the error at the location where TInst was expected

@SomeGuyWhoLovesCoding
Copy link

Is this a performance improvement thing or what?

@Geokureli
Copy link
Member Author

Is this a performance improvement thing or what?

The goal is better readability and less human-error when maintaining or editing, at the hopes of not losing any performance

@SomeGuyWhoLovesCoding
Copy link

Is this a performance improvement thing or what?

The goal is better readability and less human-error when maintaining or editing, at the hopes of not losing any performance

I'd recommend switching from typedefs to classes for QuadRectRaw, QuadColorMult, and QuadColorOffset

@Geokureli
Copy link
Member Author

Geokureli commented Jul 30, 2024

I'd recommend switching from typedefs to classes for QuadRectRaw, QuadColorMult, and QuadColorOffset

the typedefs are never instantiated, they are inlined away at compile time, as you can see in this screenshot of the completed todo item
Screenshot 2024-07-29 at 7 19 57 PM

@SomeGuyWhoLovesCoding
Copy link

I tried this pr and the rendering was actually made slower than usual on hl

@Geokureli
Copy link
Member Author

I tried this pr and the rendering was actually made slower than usual on hl

I have no plans to merge this atm, I still need to check a billion things and there's more important things to do, but if perf is worsened it means this will not ever get merged.

Also whenever you talk about performance it's always "i just ran it a bunch of times and it seems slower when I watched it". if you have an actual reproducible benchmark test, then share that, otherwise I'll assume you're just guessing

@Geokureli Geokureli modified the milestones: 5.9.0, Next Minor Version Nov 25, 2024
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.

5 participants