ruffle
ruffle copied to clipboard
avm2: Add actionscript event classes
This PR would add 40 event classes partially generated by this tool I made. This PR focuses on the classes which are simple in structure and are not much more then some properties with a clone and toString. The comments describing each part are copied from the documentation directly and may be more verbose then needed. I was unsure if all properties need a set and get; so I only created get functions for properties indicated as "read-only".
Edit - Changed number of classes to reflect PR updates
I'm not super familiar with the new AS classes process, but I believe you need to add these to https://github.com/ruffle-rs/ruffle/blob/master/core/src/avm2/globals/globals.as.
Almost all .as files added end with two ^M characters - which is the Carriage Return AFAIK. They don't cause any harm, just aren't that nice.
Thanks for pointing these out, should now be fixed
Thank you for pointing out the syntax errors, this latest commit would not build on my computer due to lack of known types namely Error, Dictionary, NetStream, ByteArray, DRMDeviceGroup, DRMContentData, DRMVoucher, Date, GameInputDevice, File, MediaPromise, SQLError, StorageVolume, and IIMEClient; also Vector in ShaderEvent has been edited to remove the <Number>. If people have any ideas about what to do I would be appreciative; my two ideas are that I could be importing them incorrectly or they haven't been implemented yet.
It looks like some of these have been implemented in rust but not exposed to actionscript, if I am understanding things correctly.
As you mentioned in your latest comment some are implemented with Rust. To allow the Actionscript classes to recognize them, add them to https://github.com/ruffle-rs/ruffle/blob/master/core/src/avm2/globals/stubs.as. I do not believe the ones that have not been implemented can be used unfortunately, but I may be mistaken.
See https://github.com/ruffle-rs/ruffle/commit/5817c1761b96fa0cfd99c1eed9277f31f68195b2 for an example of adding a stub
A couple of event classes (for example DeviceRotationEvent) only have AIR mentioned as a runtime supporting them. I don't think these ones are in the scope of Ruffle (yet)...?
Of the ones commented out, the following are AIR-only:
- DRMAuthenticateEvent
- GameInputEvent
- InvokeEvent
- MediaEvent
- NativeProcessExitEvent
- SQLErrorEvent
- ServerSocketConnectEvent
- StorageVolumeChangeEvent
Of the ones not commented out, the following are AIR-only:
- BrowserInvokeEvent
- DNSResolverEvent
- DatagramSocketDataEvent
- DeviceRotationEvent
- FileListEvent
- LocationChangeEvent
- NativeWindowBoundsEvent
- NativeWindowDisplayStateEvent
- OutputProgressEvent
- RemoteNotificationEvent
- SQLEvent
- SQLUpdateEvent
While I don't think there would be too much harm in keeping them, they are currently completely useless, since they would never appear in any non-AIR files.
See the classes with the AIR-only symbol on https://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/events/package-detail.html.
Edit: You could get all the non-AIR ones by doing document.querySelectorAll(".summaryTable tr[runtime*='Flash'] .summaryTableSecondCol a").forEach((el) => console.log(el.href)) on https://help.adobe.com/en_US/FlashPlatform/reference/actionscript/3/flash/events/package-detail.html. If you decide to do something similar to the script you did for this for other packages, hopefully that gives you a jumping off point for adjusting your Python scripts.
Yeah, they would only be unnecessary noise IMHO. Thanks for checking them @danielhjacobs!
Thanks for letting me know! I wasn't sure if AIR was going to be supported so I left it in. I'll make those adjustments
It's not out of the question for AIR to one day be supported, but it's not within the current or near-future scope.
I know it's something Mike (repo owner and creator of the Ruffle project) has said he wants to support someday:
I do want to support the AIR APIs someday, supporting all of the AIR games on Steam is a big use case I have in mind The HTMLLoader stuff will be tricky, though. More realistic would be to load the SWFs for each game directly via a standard Loader This might take some changes to the SWFs, though, as Loader isn't sandboxed as perfectly as creating a completely separate HTML embed
He said this on the #sponsors channel on Discord (only visible to sponsors of the project, mods, and people who have been granted the developer role - usually granted after someone has had a few PRs merged from what I've seen).
Edit: Basically, I'd just say to keep that commit you have that removes the AIR-only classes (just as you have done), and don't force-push things together. That way it's easy to revert the removal in the future if/when we start trying to support AIR classes.
This might be nit-picking, and I understand that this is due to the generated nature of the code, but still, looking at the final result:
I feel like the [static] and [override] attributes don't have to be duplicated in the comments when they are also there in the code as keywords in the same line (or the one after).
I agree, and have made the edits
I'm not trying to be a downer all the time, but TBH I'm not a fan of adding a bunch of unused files (those referring to unknown types), and commented out include lines for them either. 😶
Don't get me wrong, I really appreciate the effort you put into this, and even in the tool you did it with, and it will be really nice to have a bunch of missing classes defined in one swell foop. There are a couple of them that several pieces of content I'd like to see working have been missing! But we should be careful not to put quantity over quality.
I'm open to be voted down on this though. Maybe we should take the same approach as with AIR-only classes: Removing them in a separate commit, so it can be easily reverted in the future.
I wouldn't say you're being a downer, for these classes the code is relatively easy to recreate so I am happy to split this apart into several PRs as ruffle becomes more mature. I am going to have to do that anyway as this PR originally didn't have every event inside it, just the ones I could easily create at the time. The feedback is also useful for improving my generation tool and myself as a new contributor. That being said, I can reorganize the commits for this PR in this way:
- avm2: Add actionscript event classes
- Remove AIR-Only Symbols
- Remove classes which use unimplemented types
I would personally suggest that reorganization of code exactly as you described. That said, I'd like another opinion, since I rarely contribute to core. I also noticed two things:
- You commented out ShaderEvent.as because of BitmapData being an unknown type, but https://github.com/ruffle-rs/ruffle/blob/master/core/src/avm2/globals/flash/display/bitmapdata.rs does exist (though it lacks a draw method) so it may be possible to stub it.
- Depending on the order PRs get merged, you may be able to add back AsyncErrorEvent.as if the brand new PR https://github.com/ruffle-rs/ruffle/pull/7633 gets merged before this one, since it adds Error.as
Additionally, I'm unsure if other developers may request tests be added for some of these classes. Tests are one of the larger barriers to entry for new contributors, and they can theoretically be added in a follow-up PR, so you may not need to add them now depending what other developers think. You can see https://github.com/ruffle-rs/ruffle/blob/HEAD/CONTRIBUTING.md#test-guidelines for information about adding tests.
That said, perhaps tests are overkill for purely ActionScript-based classes.
Good observations, @danielhjacobs!
Additionally, I'm unsure if other developers may request tests be added for some of these classes.
Also great point, but IMHO these event classes are so simple, being almost "plain old data"-like, having only a bunch of properties and no real functionality in themselves, that unit/regression tests are not really useful for them.
What I think should be done, is a manual overview of all the classes, to see if everything is as it should be in them. But that's just my pair of pennies.
The only thing that I think needs attention is whether to keep GestureEvent; as I could copy the MouseEvent Code wholesale, but wouldn't know how to actually attach it to a gesture input - if that's necessary, if not then I can just copy the code and leave it at that.
Following advice from the discord, the stageX and stageY getters inside GestureEvent are borrowed from MouseEvent. There aren't any other issues that I can immediately see
Do you think we should maybe add a little note in a comment at the top of every newly added, autogenerated file, stating something like "this file was autogenerated from the official AS3 reference at <URL> by <toolname>"? And maybe that "However, it won't be regenerated in the future, so feel free to edit/fix"?
I think the formatting of the code could use some adjustments (I saw a bit too many empty lines for my taste), but I guess that can be addressed together with #7666, if this is merged first.
Also, any potential funky business (bugs, omissions) in clone or toString (like the ones mentioned about MouseEvent on Discord) can also be taken care of once they are discovered, and are causing any issues in practice.
Was DRMStatusEvent omitted because of the too many AIR-only properties?
@torokati44
Do you think we should maybe add a little note in a comment at the top of every newly added, autogenerated file, stating something like "this file was autogenerated from the official AS3 reference at
<URL>by<toolname>"? And maybe that "However, it won't be regenerated in the future, so feel free to edit/fix"?
Yes, here is the message I currently thought up:
// The initial version of this file was autogenerated from the official AS3 reference at <url> by https://github.com/golfinq/ActionScript_Event_Builder
// It won't be regenerated in the future, so feel free to edit and/or fix
I think the formatting of the code could use some adjustments (I saw a bit too many empty lines for my taste), but i guess that can be addressed together with https://github.com/ruffle-rs/ruffle/pull/7666, if this is merged first.
Agreed, if that does get merged first then this PR will be changed to reflect the recommendations the linter will make.
Also, any potential funky business (bugs, omissions) in clone or toString (like the ones mentioned about MouseEvent on Discord) can also be taken care of once they are discovered, and are causing any issues in practice.
This makes me feel that at the moment, until issues are encountered, I should stick with my first guesses about the toString arguments and parameter order and for the moment just work on adding that comment above?
Was DRMStatusEvent omitted because of the too many AIR-only properties?
Yes, after removing the AIR-only properties there wasn't much left and felt like it was better to leave out; that said I am not iron clad on this and can add it back in
Also as part of the next rebase/confict resolution I am thinking of putting the include ...\events\... in globals.as back into alphabetical order
Yes, here is the message I currently thought up:
That sounds about right. Maybe the first line could be split into two somewhere, but this is also nitpicking... :)
This makes me feel that at the moment, until issues are encountered, I should stick with my first guesses about the toString arguments and parameter order and for the moment just work on adding that comment above?
Yeah, I personally think this is fine. But if anyone else (of any other reviewers) feels like this is too negligent, feel free to override me on this.
Also as part of the next rebase/confict resolution I am thinking of putting the include ...\events... in globals.as back into alphabetical order
Yeah, that would be good.
Sorry, I am not sure why it says it's out of date; but I ended up updating and force-pushing twice.
Applied some minor fixes but on my local machine I am getting this error when trying to build
error: no rules expected the token `(`
--> core\src\avm2\globals.rs:725:13
|
691 | macro_rules! avm2_system_classes_playerglobal {
| --------------------------------------------- when calling this macro
...
725 | ("flash.geom", "Matrix", matrix),
| ^ no rules expected this token in macro call
Which I don't believe is connected to this PR
EDIT: Nevermind, it is and I found it. Fixed and built on my machine