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

avm1: Reference display objects indirectly by string path #9447

Merged
merged 32 commits into from
Mar 1, 2023

Conversation

CUB3D
Copy link
Contributor

@CUB3D CUB3D commented Feb 7, 2023

In AVM1, MovieClips are referenced by their path on the stage, which causes some differences in how they are handled compared to Objects, namely:

  • Removing a clip causes its properties to become Undefined and the clip itself to become "" (empty string)
  • Re-creating another clip at the same path, causes existing references to "revive" (their properties become valid again)

This pr takes a different approach to #5492, as per https://web.archive.org/web/20051224124649/http://moock.org:80/asdg/technotes/movieclipDatatype/:
"Movie clips are implemented separately from objects internally in the Player, ..."

The combination of this strange behavior and clips having a unique result for typeof makes me think that this quote should be taken literally. This pr starts to separates Value::MovieClip from Value::Object, this new variant stores the path to the targeted object. When coerced to a String, this variant results in the path if the object is present on the stage, or an empty string if it is not. When coerced to an Object the result is either Undefined if the clip has been removed or the backing Object if it is not.

(Note that in order to simplify this, Value::Object is still used internally, and is only converted to a Value::MovieClip when pushed onto the avm stack, this isn't clean but it's a lot simpler. I don't intend for this "hack" to be permanent)

(This pr also fixes a minor issue with SuperObject string coercion found while creating tests)

TODO:

  • Add more tests
  • Why does 9052 preloader no longer work (unload handlers, fixed)
  • Why does nothing spawn in 1017 (SuperObject)
  • Why is mochi loader borked (unload handlers, fixed)
  • Bloons (mochi)
  • Why is 6203 / 8872 broken (broke SuperObject)
  • Monkey kick off
  • Nanaca Crash
  • Numballs
  • Ninja painter???????? (DO caching)
  • Cube Mayhem? (DO caching)
  • Cyber chaser (DO caching)
  • Kodachrome (audio loop is back)
  • Extreme Pamplona (DO caching)
  • SMF2 (DO caching)
  • Super Pang (This is the same scoping bug as wuzi chess with more indirection)
  • Bubble Bobble (events on removed clips)
  • Golphysics (unintentionally fixed?)
  • 1029 (swfv5 buttons)
  • Wuzi chess (scoping)
  • 4088 (Going to consider this out of scope for this pr, its big enough)
  • 1561 (going to consider this out of scope for now)
  • Refactor
  • Dragon Fable regression (more resolving)

Deep Breath
Fixes #993, fixes #2840, fixes #1513, fixes #1140, fixes #5275, fixes #4793, fixes #4404, fixes #9257, fixes #9135, fixes #5923, fixes #2575, fixes #8765, fixes #7711, fixes #5711, fixes #5095, fixes #7169, fixes #7345, fixes #7362, fixes #6960, fixes #6203, fixes #8872, fixes #1017, fixes #3839, fixes #3604, fixes #6185, fixes #3916, fixes #6336, fixes #4118, fixes #9504, fixes #1029, fixes #2732, fixes #5277, fixes #8576, fixes #1989, fixes #3118, fixes #9514, fixes #9556, fixes #8857, fixes #8314, fixes #8463, fixes #7728, fixes #7846, fixes #7928, fixes #8498, fixes #8784, fixes #9800
(please link anything else that has the same bug as 993 and isn't in this list or the todos)

@n0samu
Copy link
Member

n0samu commented Feb 7, 2023

Although the game is far from working, this fixes the panic in #4932! 🎉
This is going to be so exciting for so many people 😃

@n0samu
Copy link
Member

n0samu commented Feb 7, 2023

Unfortunately, Desktop Tower Defense seems not to be the only game with a Mochi preloader that's no longer working. Bloons TD is also affected.

@n0samu
Copy link
Member

n0samu commented Feb 7, 2023

Testing with a version of Desktop Tower Defense modified not to use a Mochi loader, #8765 is fixed!
And many, many other tower defense games with similar issues are also fixed! We'll be able to close all of these issues:
#7711, #5711, #5095, #7169, #7345, #7362, #6960.

On the other hand, Trojan War (#6203) seems to have regressed further. You can no longer place towers, and the panels on the top-left and bottom-right display incorrectly: (Fixed!)
image
image

And Frontline Defense (#8872) has largely the same problems as Trojan War. You can't place towers and some info in textboxes shows up as "undefined". (Fixed!)

@n0samu
Copy link
Member

n0samu commented Feb 7, 2023

Testing the games in my collection, here are some more regressions I'm noticing:

  • In Ninja Painter, after you grab the paint, a flashing rainbow shape appears in the top-left corner of the screen, and slowly grows larger as time goes on, making the game very unpleasant to play. And the same problem also occurs in Ninja Painter 2. (Fixed!)
  • In Cube Mayhem, nothing happens when the cube hits the goal tile, so levels are impossible to complete. (Fixed!)
  • In Cyber Chaser, bullets stop after traveling barely any distance, making it impossible to shoot the boxes or enemies. And there are also bullets traveling across the top of the screen, for some reason. (Fixed!)
  • Monkey Kick Off shows a blue box near the top-left corner of the screen. (Fixed!)
    image
  • In Nanaca Crash, after you click the Play button on the main menu it enters an infinite loop, so the game is unplayable. (Fixed!)
  • Numballs gets stuck on the loading screen with this error: (Fixed!)
    ERROR ruffle_core::avm1::globals::shared_object: SharedObject::get_local: Invalid character in name

@n0samu
Copy link
Member

n0samu commented Feb 7, 2023

The SWF provided in #3839 seems to log the correct output now. Some related issues are also fixed: #3604, #6185

But some are not fully resolved yet:

core/src/avm1/value.rs Outdated Show resolved Hide resolved
@ousia
Copy link
Contributor

ousia commented Feb 8, 2023

@CUB3D,

many thanks for your PR. It is really great news!

I wonder whether #1561 and #6581 will work with this PR (I’m afraid I don’t have a way to check it).

Many thanks for your work again.

@Lord-McSweeney
Copy link
Collaborator

Relevant: #6548 (comment)

@n0samu
Copy link
Member

n0samu commented Feb 9, 2023

I can confirm that Trojan War, Frontline Defense, Monkey Kick Off, Nanaca Crash, and Numballs are working great with the latest changes! 🎉

@ousia No, those two issues are not fixed. They seem to have somewhat different root causes that would likely need to be solved separately from this PR.
Your question also reminded me to check #8213. It's not fixed either, so I guess our theory about the root cause of the problem was incorrect.

@n0samu
Copy link
Member

n0samu commented Feb 9, 2023

@CUB3D your latest commit seems to have broken most of the tower defense games again to some degree. Vector TD is back to being totally broken, for instance.
Also, the "Mario dupe" issue in SMF2 has returned, and so has the Electricman panic 😦
And the softlock after losing in Extreme Pamplona is back too.

@ousia
Copy link
Contributor

ousia commented Feb 9, 2023

@n0samu, many thanks for your fast reply.

I thought #5492 was about indirect object references.

So, it seems that some issues with indirect references will need further investigation after this PR is merged.

Many thanks again for your help (and to @CUB3D for the PR).

@Lord-McSweeney
Copy link
Collaborator

Lord-McSweeney commented Feb 10, 2023

Could the fix for #1029 (already implemented in #5492) also be implemented in this PR? It fixes at least one issue and is relatively simple to implement.

Also, has this been tested with #6906? I feel like it would be a good test case, seeing as it's very buggy right now.

@n0samu
Copy link
Member

n0samu commented Feb 10, 2023

Super Mario 63 is a whole other can of worms that we probably shouldn't touch yet 😆

@Lord-McSweeney
Copy link
Collaborator

Toad06 commented that some issues may occur because of #3839, and it's one of the more complex AVM1 games, so it would be an interesting test case.

@n0samu
Copy link
Member

n0samu commented Feb 10, 2023

@CUB3D Thanks so much for all your work on this! I just retested all of the games I tested previously, and found that everything is working great except for Wuzi Chess (which I know you're working on) and Bubble Bobble (#6185, which you might have missed).

@n0samu
Copy link
Member

n0samu commented Feb 10, 2023

I'm seeing a regression in Golphysics (#5219). Now when I click the Start button to start the game, it goes to a blank screen instead.
WARN run_frame: ruffle_core::avm1::globals::movie_clip: MovieClip.swapDepths: Invalid target -- in [Actions Parent] / [Frame]

Also, I just remembered to test #2732 and that issue is still occurring the same as before. Not sure if that issue is really related though.

@Lord-McSweeney
Copy link
Collaborator

This also fixes #9504.

@ousia
Copy link
Contributor

ousia commented Feb 10, 2023

Could the fix for #1029 (already implemented in #5492) also be implemented in this PR? It fixes at least one issue and is relatively simple to implement.

If #1029 could be fixed, in #1561 (comment) @Herschel stated that #1561 is a variant of it (although it needs work to consider graphics as well).

Just in case it could be considered in this or in a future PR.

@danielhjacobs
Copy link
Contributor

The fixes keyword is picky, but can be used to auto-close issues when used properly. You should probably copy paste the below to auto-close all the issues, as the current phrasing will only close the first listed issue automatically:

Fixes #993, fixes #2840, fixes #1513, fixes #1140, fixes #5275, fixes #4793, fixes #4404, fixes #9257, fixes #9135, fixes #5923, fixes #2575, fixes #8765, fixes #7711, fixes #5711, fixes #5095, fixes #7169, fixes #7345, fixes #7362, fixes #6960, fixes #6203, fixes #8872, fixes #1017, fixes #3839, fixes #3604, fixes #6185, fixes #3916, fixes #6336, fixes #4118, fixes #9504

@n0samu
Copy link
Member

n0samu commented Feb 11, 2023

With the latest commits, #2732 is actually fixed! 🎉

Also, I can confirm that Bubble Bobble is fixed and the Golphysics regression is resolved. 👍

@Lord-McSweeney
Copy link
Collaborator

Herschel commented on #4088 that #1029 is more complicated; sometimes, the behavior is present in SWFv6 and above. Currently, this PR does not fix #6581, #5579, or #2181, but that might be better kept for another PR.

@Toad06
Copy link
Member

Toad06 commented Feb 11, 2023

Also fixes #5277 and #8576.
#1989 looks good too.

@n0samu
Copy link
Member

n0samu commented Feb 13, 2023

I can confirm that Wuzi Chess is fixed! 😄
image

And #9514 is fixed as well!

@CUB3D CUB3D force-pushed the avm1_mc_string_paths branch from 2206902 to 9153012 Compare February 13, 2023 15:33
@CUB3D CUB3D marked this pull request as ready for review February 13, 2023 16:26
@@ -544,18 +546,30 @@ impl<'a, 'gc> Activation<'a, 'gc> {
}
}

fn stack_push(&mut self, mut value: Value<'gc>) {
if let Value::Object(Object::StageObject(s)) = value {
Copy link
Member

@Aaron1011 Aaron1011 Feb 13, 2023

Choose a reason for hiding this comment

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

Do we need the ability to push to the AVM1 stack without this logic running? If not, could we move this logic into the push method, and have it take an Activation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really need to be able to push without this running, so moving it would be possible, but because we need a mutable borrow of activation inside of push to create a MovieClipReference as well as a mutable borrow of self.context.avm1 to call push it would likely be a much more invasive change

Copy link
Member

Choose a reason for hiding this comment

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

Since this is a pretty subtle issue, I'd like to make it harder to accidentally skip running this logic (by using avm1.push instead of stack_push). However, I don't want to block merging this PR, so I think this could be done as a follow-up refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just adding that the only solution I can think of, would be either removing avm1.push and switching everything to use activation.push or using interior mutability so that avm1.push can take &self over &mut self. Did you have another solution in mind?

Copy link
Collaborator

@adrian17 adrian17 Feb 15, 2023

Choose a reason for hiding this comment

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

I'd like to make it harder to accidentally skip running this logic (by using avm1.push instead of stack_push).

On the contary, I'd consider minimizing complexity of push whenever it's not needed (say action_add). In avm2, stack push keeps popping up on profiles with non-trivial %, while on paper it should be one of the cheapest things ever.

@nachtklavier
Copy link

@CUB3D Thank you very much. Canyon Defense works like a charm now. Highly appreciate your work!

@TheGhostboi64
Copy link

This now fixes the problem the swf i used! Good job and I wish y'all the best of luck on getting ruffle completed!

@Herschel
Copy link
Member

Thanks, @CUB3D !

@Kippykip
Copy link

Kippykip commented Apr 13, 2023

Not sure if this is related but it seems this issue has semi fixed the invisible ragdoll issue in Free Rider 2
#4817
The ragdoll now appears but in a trailing effect?
image_2023-04-13_230116696

@n0samu
Copy link
Member

n0samu commented Apr 13, 2023

Please add that as a comment to the original issue, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment