Skip to content

avm2: Move DisplayObject assignment into an initializer #11084

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 27, 2023
Merged

avm2: Move DisplayObject assignment into an initializer #11084

merged 1 commit into from
May 27, 2023

Conversation

Aaron1011
Copy link
Member

@Aaron1011 Aaron1011 commented May 17, 2023 •
edited
Loading

No description provided.

@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from 8dc9eb2 to fa6c168 Compare May 17, 2023 05:47
@Lord-McSweeney
Copy link
Collaborator

Progresses #11051.

Lord-McSweeney
Lord-McSweeney reviewed May 17, 2023
View reviewed changes
@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from fa6c168 to 3b4e0e1 Compare May 18, 2023 03:01
@Aaron1011 Aaron1011 added the waiting-on-review Waiting on review from a Ruffle team member label May 18, 2023
@iwannabethedev iwannabethedev mentioned this pull request May 19, 2023
Closed
@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch 2 times, most recently from 4b93adf to 07f1bae Compare May 20, 2023 00:39
@Lord-McSweeney Lord-McSweeney mentioned this pull request May 20, 2023
Closed
@n0samu n0samu mentioned this pull request May 21, 2023
Closed
@Aaron1011 Aaron1011 requested review from kmeisthax and adrian17 May 21, 2023 14:34
@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from 8810763 to 7b31eeb Compare May 21, 2023 18:25
Lord-McSweeney
Lord-McSweeney reviewed May 21, 2023
View reviewed changes
core/src/avm2/globals/flash/media/sound.rs
@@ -19,8 +19,6 @@ pub fn init<'gc>(
args: &[Value<'gc>],
) -> Result<Value<'gc>, Error<'gc>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should Sound also be moved to an instance allocator?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a test - it looks like Sound is a 'normal' object (the initialization happens during the super() call, not during the allocator).

@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from 7b31eeb to 894438f Compare May 21, 2023 18:56
@SN902
Copy link

SN902 commented May 21, 2023

I found regression, this commit brings back flickering buttons/sliders in #10208 and PortageCS5.zip

kmeisthax
kmeisthax reviewed May 22, 2023
View reviewed changes
Copy link
Member

@kmeisthax kmeisthax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure why super_init got removed from Sound and ApplicationDomain - is there some subtlety of initialization I'm missing? I thought they were "normal" (i.e. no native-allocated fields). I try to keep super_init in native code since the AS3 compiler automatically inserts constructsuper everywhere anyway, but this isn't particularly important with EventDispatcher being lazy.

The regression @SN902 found is worth looking into but I won't be able to do that for a few more days. Looking at the original PR #10734 it looks like you correctly carried forward everything from that, so it SHOULDN'T be broken, which suggests that we might need to move the check somewhere else, or set a flag here that play/goto checks for.

core/src/avm2/globals/flash/display/bitmap_data.rs
} else {
if character.is_some() {
//TODO: Determine if mismatched symbols will still work as a
//regular BitmapData subclass, or if this should throw
Copy link
Member

Choose a reason for hiding this comment

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

My gut feeling is that it should throw, but I'm not going to ask for it to throw until/unless we have tests that go that way.

core/src/display_object/graphic.rs Outdated
context: &mut UpdateContext<'_, 'gc>,
avm2_object: Avm2Object<'gc>,
) -> Self {
pub fn new_with_avm2(context: &mut UpdateContext<'_, 'gc>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

This title has become a bit of a misnomer now that it just constructs an empty graphic without taking an AVM2 object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed this to empty

core/src/display_object/loader_display.rs Outdated
avm2_object: Avm2Object<'gc>,
movie: Arc<SwfMovie>,
) -> Self {
pub fn new_with_avm2(activation: &mut Activation<'_, 'gc>, movie: Arc<SwfMovie>) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed this to empty

@Aaron1011
Copy link
Member Author

Aaron1011 commented May 22, 2023 •
edited
Loading

@kmeisthax: The 'super_init' removes were drive by fixes. Those calls were in init methods, which are just normal methods called from the constructor (the super_init call was left over when we defined the class in actionscript).

I discussed the regression on Discord, but forgot to leave a comment on the pr- I fixed it in ad15328 (#11084)

@n0samu
Copy link
Member

n0samu commented May 22, 2023 •
edited
Loading

With this PR, #8947 is occurring again 😦

ERROR run_frame:run_all_phases_avm2: ruffle_core::avm2::events: Error dispatching event EventObject(EventObject { type: "addedToStage", class: flash.events::Event, ptr: 0x218a7850388 }) to handler FunctionObject(FunctionObject { ptr: 0x218a27e0a88 }) : TypeError: Error #1009: Cannot access a property or method of a null object reference. (accessing field: visible)
        at classes::Level/holderOnAddedToStage()
        at flash.display::DisplayObjectContainer/flash::display::DisplayObjectContainer::addChildAt()
        at classes.app::Main/createLevel()
        at classes.app::Main/nextLevel()
        at classes::ShutterIn/onEnterFrame()

Note: To test this game on desktop you'll need to merge in #11159 because I broke left/right arrow key controls on desktop with my text control PR (sorry!)

@n0samu n0samu mentioned this pull request May 23, 2023
Closed
@KrzysztofoGuard
Copy link

KrzysztofoGuard commented May 24, 2023 •
edited
Loading

Tell me, will the "Super goblin war machine" game I submitted for revival to the Ruffle team will work, or have you already become discouraged? I realize there have been any problems recently, so please give me a specific answer.

@n0samu
Copy link
Member

n0samu commented May 24, 2023

@KrzysztofoGuard The answer to your question is here #11051 (comment). There some more things we need to figure out before the game will work at all.

@n0samu
Copy link
Member

n0samu commented May 24, 2023

I can confirm that with #11195 also merged in, Pursuit of Hat no longer regresses!

@n0samu n0samu mentioned this pull request May 25, 2023
Merged
@waspennator
Copy link

Should close #8409 with pony towers being unable to damage targets.

@SN902
Copy link

SN902 commented May 26, 2023 •
edited
Loading

Found regression V8MuscleCars.zip after intro this game stuck on black screen with ERROR ruffle_core::avm2::events: Error dispatching event EventObject(EventObject { type: "enterFrame", class: flash.events::Event, ptr: 0x2a000207ba8 }) to handler FunctionObject(FunctionObject { ptr: 0x2a07acda048 }) : TypeError: Error 1009: Cannot access a property or method of a null object reference. (accessing field: numChildren)

@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from 9c61410 to 6c698cf Compare May 26, 2023 23:13
@Aaron1011
Copy link
Member Author

@SN902 I pushed a new commit which should fix the regression

@SN902
Copy link

SN902 commented May 26, 2023 •
edited
Loading

@SN902 I pushed a new commit which should fix the regression

Fixed

@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from 6c698cf to 07c09f0 Compare May 27, 2023 00:57
@Aaron1011 Aaron1011 force-pushed the do-initializer-final branch from 07c09f0 to b2b1dff Compare May 27, 2023 02:43
@Aaron1011 Aaron1011 merged commit aeb089a into ruffle-rs:master May 27, 2023
@Lord-McSweeney Lord-McSweeney mentioned this pull request May 28, 2023
Closed
@MartySVK MartySVK mentioned this pull request May 31, 2023
Closed
@Lord-McSweeney Lord-McSweeney mentioned this pull request Jun 5, 2023
Closed
@n0samu n0samu mentioned this pull request Jun 6, 2023
Closed
@Lord-McSweeney Lord-McSweeney removed the waiting-on-review Waiting on review from a Ruffle team member label Jun 14, 2023
@n0samu n0samu mentioned this pull request Jul 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmeisthax kmeisthax kmeisthax left review comments

@Lord-McSweeney Lord-McSweeney Lord-McSweeney left review comments

@Dinnerbone Dinnerbone Dinnerbone approved these changes

@adrian17 adrian17 adrian17 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Aaron1011 @Lord-McSweeney @SN902 @n0samu @KrzysztofoGuard @waspennator @kmeisthax @Dinnerbone @adrian17