openfl / lime

A foundational Haxe framework for cross-platform development
https://lime.openfl.org/
MIT License
749 stars 359 forks source link

Nightly Haxe issues #1763

Closed Geokureli closed 4 months ago

Geokureli commented 4 months ago

Just mentioning this here in case it doesn't solve itself.

Flixel CI has started failing on nightly haxe versions due to lime's shadowed implementation of haxe.io.Bytes with the following errors:

Cpp/Neko:

/.../_internal/macros/AssetsMacro.hx:41: characters 26-33 : Too many arguments

HL:

/.../AssetsMacro.hx:78: characters 12-19 : hl.Bytes should be Int for optional function argument 'length'

HTML5:

/.../AssetsMacro.hx:37: characters 12-26 : js.lib.ArrayBuffer should be Int for optional function argument 'length'

the OG haxe.io.Bytes was changed for the first time in 5 years, all they did was remove #if cs sections, so I'm not sure why that would cause these specific errors

My main concern is that we don't really know why lime has it's own Bytes class, so if things do change with Bytes, lime will need to manually make those same changes. I wonder if it's simply better to create a lime.io.Bytes and use that? I also don't see why lime's Bytes has a different constructor for js

Geokureli commented 4 months ago

Also note: haxe.Timer is also shadowed, according to Zeta: "the proper way to make Timer work would be to integrate the haxe event loop into the Lime event loop." here's how heaps does it

joshtynjala commented 4 months ago

haxe.Timer is also shadowed

I noticed this one recently too. In fact, I discovered that Lime's custom haxe.Timer implementation causes openfl.utils.Timer to behave differently than flash.utils.Timer. Lime limits haxe.Timer to the frame rate, but flash.utils.Timer is allowed to update more frequently than flash.events.Event.ENTER_FRAME is dispatched.

In Flash, you can set the stage frame rate to a low value, like 1 FPS, but then call TimerEvent.updateAfterEvent() to force a rendering update on demand. Because Lime and OpenFL's Timers are limited to the frame rate, this (admittedly advanced) use case is not possible in OpenFL, like it is in Flash.

player-03 commented 4 months ago

So I think the immediate problem is that Lime's Bytes implementation checks #if js to determine what constructor to use, but AssetsMacro checks #if html5. I don't know why this is coming up now in particular, but it was only a matter of time.


I went back to when Lime's version of the class was added. The commit created the JS-only version of the class with a bunch of differences copied over Haxe's JS-specific version of the class.

player-03 commented 4 months ago

I went through the various versions of haxe.io.Bytes to find everything Lime changed. I left out changes that have no effect (e.g., whitespace changes), and ones where Haxe's version is more up-to-date.

That leaves WebAssembly as the tricky part. If not for that, we could simply remove Lime's version of the file.

Perhaps we could override Bytes.hx specifically for that target? Point to "templates/webassembly/src" as an additional source directory, and add a custom haxe/io/Bytes.hx in there? Not a huge fan of the idea, though. We'd probably forget it was there, and it'd fall even further behind.

player-03 commented 4 months ago

I don't know why this is coming up now in particular, but it was only a matter of time.

There was a change to the macro context just a couple days ago. Something to do with caching. Perhaps you were never supposed to be able to check #if html5 during a macro, but Lime got away with it because it happened to be cached.

Geokureli commented 4 months ago

in the short term, does this mean that a quick fix is to change AssetMacro's #if html to #js? would this create issues for other js targets like node or something?

player-03 commented 4 months ago

does this mean that a quick fix is to change AssetMacro's #if html to #js?

It turns out that won't work. See my PR for details.

player-03 commented 4 months ago

(We could patch this via macro if it's important.)

Actually it seems we can't. Classes marked @:coreApi are checked afterwards, and if any API changes are detected, compilation fails.

If we wait until runtime, there's an alternate technique that does work.

That said, I'm starting to doubt that Bytes ever actually needed a setter. Only ByteArray is documented as having a setter, and it's an abstract, so there's no patching needed.

player-03 commented 4 months ago

Merged. If you set up the tests to get OpenFL Lime from Git, they should hopefully work again. Let us know if not!

Geokureli commented 3 months ago

Merged. If you set up the tests to get ~OpenFL~ Lime from Git, they should hopefully work again. Let us know if not!

Seems Flixel CI is still failing with nightly haxe even with lime 8.1.2

/haxelib/lime/8,1,2/src/lime/_internal/macros/AssetsMacro.hx:36: characters 32-39 : Too many arguments /haxelib/lime/8,1,2/src/lime/_internal/macros/AssetsMacro.hx:41: lines 41-44 : Missing super constructor

player-03 commented 3 months ago

I can't reproduce this locally, can you?

Geokureli commented 3 months ago

I wonder if it's linux specific, since Flixel CI uses linux. I'll try on my dual-boot laptop

player-03 commented 3 months ago

I run Linux.

Geokureli commented 3 months ago

Just to be certain, can you check this and call out any differences or mistakes? also are you using the develop branch or the actual release?

I'll report back after trying nightly with lime 8.1.2, in a bit

player-03 commented 3 months ago

Oh interesting, there's still a difference between Haxe 4.3.4 and nightly. I'll keep digging.

player-03 commented 3 months ago

Ok, I don't fully understand what's going wrong, but the nightly build definitely changes something. I tried hard-coding different super() calls, and none of them worked. If we pass two arguments, that's too many arguments. If we pass one argument, it isn't enough arguments and it expected a different type. Clearly it's somehow using both the html5 version of Bytes and the hl version, at different points in the compilation. It shouldn't be doing this, but to be fair, we shouldn't have shadowed the class.

openfl/lime#1765 fixes the issue by removing our version of the class and falling back to the standard library's version. Fortunately, though unsurprisingly, the Haxe devs made sure the standard library works.

We'll have to merge that PR by the time Haxe 5 comes out.

Geokureli commented 3 months ago

Thanks for all you've done!