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

Make ByteArray compatible with Haxe's Bytes class. #2692

Merged
merged 6 commits into from
Feb 29, 2024

Conversation

player-03
Copy link
Contributor

Currently, Lime shadows haxe.io.Bytes, but we might not have to. I'm trying to rewrite any piece of code that references the shadowed version of the class to refer to the original version instead.

ByteArrayData is one such piece of code. It includes lots of references to the private variable l, which only exists in Lime's version of the class.

For whatever reason, openfl-js wants length to be a property with a getter and setter. But it also wants to be able to bypass those accessors. But it also doesn't want to use Haxe's accessors that can be bypassed using @:bypassAccessor; it calls Object.defineProperties() at runtime. So it stores the value in l, and uses l when it wants to bypass the accessors.

It all works, of course. But I can't seem to find any reason for any of it. ByteArray is the public API for all this, and it defines a perfectly functional getter and setter. There's no need for ByteArrayData to have its own setter on top of that. Is there? (I'll admit I've never actually used openfl-js, but I did make sure it still compiles after these changes.)

With one exception for the time being.
@skial skial mentioned this pull request Feb 27, 2024
1 task
I spent at least an hour trying to figure out why it wasn't the same as `_length`. Hopefully this comment can save the next person that trouble.

For background information, see https://en.wikipedia.org/wiki/Dynamic_array.
@player-03
Copy link
Contributor Author

player-03 commented Feb 27, 2024

Whoops, a9c080e is counterproductive. I was trying to bypass the overridden set_length(), which I'd already deleted. The base set_length() literally just sets l, providing backwards compatibility. Force pushing to get rid of it.

@player-03
Copy link
Contributor Author

This PR has to be merged before openfl/lime#1765. I made sure that this is compatible with either version of Lime; even if we never merged that one, this shouldn't break anything.

If it did break anything, it would only be for OpenFL-JS users, not regular OpenFL users. And only if the OpenFL-JS users were messing around with non-public APIs.

@joshtynjala
Copy link
Member

I did a quick test with openfl-js, and this breaks the length property when set with JavaScript code.

The following JS code should print 0 to the console three times. It worked previously, but with your changes, it throws an exception.

var bytes = new openfl.utils.ByteArray();
bytes.length = 3;
for (let i = 0; i < 3; i++)
{
    console.log(bytes.readByte());
}

While ByteArrayData is a non-public API in Haxe, it is actually a public API in JS because ByteArray is a Haxe abstract. Abstracts exist at compile-time only, so JS is working directly with ByteArrayData. It needs a length setter to ensure that __resize is called.

@player-03
Copy link
Contributor Author

I guess that explains why it was added. I'll see if I can work out another option.

@player-03
Copy link
Contributor Author

Ok, how's this look?

@joshtynjala
Copy link
Member

I get the following errors with when building openfl-js (using Lime 8.1.1):

../node_modules/openfl-haxelib/src/openfl/utils/ByteArray.hx:1111: lines 1111-1114 : Field get_length should be declared with 'override' since it is inherited from superclass haxe.io.Bytes
../node_modules/openfl-haxelib/src/openfl/utils/ByteArray.hx:1116: lines 1116-1119 : Field set_length should be declared with 'override' since it is inherited from superclass haxe.io.Bytes

@joshtynjala
Copy link
Member

It looks like the openfl-js length property will need to be defined with methods that have a name that doesn't conflict with get_length and set_length:

"length": {
	get: ByteArrayData.prototype.get_length2,
	set: ByteArrayData.prototype.set_length2
}
private function get_length2():Int
{
	return __length;
}

private function set_length2(value:Int):Int
{
	return (this : ByteArray).length = value;
}

Then, it looks like ByteArrayData will need to use __length in more places to avoid length turning into get_length and set_length. I changed readUnsignedByte() to use __length, and that seems to work pretty well with my JS test code from above:

public function readUnsignedByte():Int
{
	if (position < __length)
	{
		return get(position++);
	}
	else
	{
		throw new EOFError();
		return 0;
	}
}

So maybe if all of the other read/write methods use __length where appropriate, it will be the right fix.

This is only temporarily necessary, to avoid the getter Lime adds to `haxe.io.Bytes`. Once that's removed, it would be safe to revert this.
These `#if openfljs` cases can be deleted once Lime stops shadowing `haxe.io.Bytes`.
@player-03
Copy link
Contributor Author

Whoops, not sure how I missed that. Guess I forgot Lime declared get_length() and set_length(), but I recall testing against both versions of Bytes. Maybe not?

In any case, this should fix it.

@joshtynjala joshtynjala merged commit 2f51090 into openfl:develop Feb 29, 2024
24 checks passed
@player-03 player-03 deleted the remove_Bytes branch February 29, 2024 16:33
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.

None yet

2 participants