Scirra / Construct-bugs

Public bug report submissions for Construct 3 and Construct Animate. Please read the guidelines then click the 'Issues' tab to get started.
https://www.construct.net
107 stars 83 forks source link

(SDK) Multiple instances, variadic parameters, variables, also confusion. (c3runtime) #2155

Closed Wackytoaster closed 5 years ago

Wackytoaster commented 5 years ago

Problem description

Hello, a user of my addon (https://www.construct.net/en/make-games/addons/163/tweeen) pointed out a bug that send me down the rabbit hole. I did manage to fix it though it feels more like a workaround. I´m pretty confused about the nature of this. Also don´t worry, I´m not gonna make you debug my addon!

The setup are 3 instances with platform behavior aswell as a slightly modified SDK template behavior. On landed, they call "Stop" with parameters A (type "any") and B (type "variadic") It is setup so that A is the Sprite.UID and B is an instance variable. So far so good.

exhibita

Now I preview the project. The stop code does nothing except this:

exhibitb

And here is where I get really confused... the log shows this.

exhibitc

So b[0] correctly outputs the variables, however, the variadic array b appears to be updated (?) to the last instance for all instances. That caused the problem with my plugin. Now I worked around this by doing

var temp = []; variadic.forEach(function(e){ temp.push(e); });

This works ok-ish. However, replacing "on landed" with "is on floor - trigger once" still causes problems anyway. (Don´t know if it´s related or not.)

Sooo... what exactly is happening? Should this happen?

Attach a .c3p

Contains sample project with bundled test addon. bugorfeature.c3p.zip

Steps to reproduce

  1. See above

Observed result

The value gets updated sort of?

Expected result

The value to stay the same I guess... I´m still confused.

Affected browsers

System details

View details Platform information Browser: Chrome Browser version: 69.0.3497.100 Browser engine: Blink Browser architecture: 64-bit Context: webapp Operating system: Windows Operating system version: 10 Operating system architecture: 64-bit Device type: desktop Device pixel ratio: 1 Logical CPU cores: 8 Approx. device memory: 8 GB User agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/69.0.3497.100 Safari/537.36 C3 release: r122 (beta) Language setting: en-US Local storage Storage quota (approx): 15 gb Storage usage (approx): 1.5 gb (10.2%) Persistant storage: Yes Browser support notes This list contains missing features that are not required, but could improve performance or user experience if supported. Nothing is missing. Everything is OK! WebGL information Version string: WebGL 2.0 (OpenGL ES 3.0 Chromium) Numeric version: 2 Supports NPOT textures: yes Supports GPU profiling: no Vendor: Google Inc. Renderer: ANGLE (NVIDIA GeForce GTX 1070 Direct3D11 vs_5_0 ps_5_0) Major performance caveat: no Maximum texture size: 16384 Point size range: 1 to 1024 Extensions: EXT_color_buffer_float EXT_texture_filter_anisotropic OES_texture_float_linear WEBGL_compressed_texture_s3tc WEBGL_compressed_texture_s3tc_srgb WEBGL_debug_renderer_info WEBGL_debug_shaders WEBGL_lose_context Audio information System sample rate: 48000 Hz Output channels: 2 Output interpretation: speakers Supported decode formats: Ogg Opus (audio/ogg; codecs=opus) WebM Opus (audio/webm; codecs=opus) Ogg Vorbis (audio/ogg; codecs=vorbis) WebM Vorbis (audio/webm; codecs=vorbis) MPEG-4 AAC (audio/mp4; codecs=mp4a.40.5) MP3 (audio/mpeg) FLAC (audio/flac) PCM WAV (audio/wav; codecs=1) Supported encode formats: WebM Opus (audio/webm; codecs=opus) Video information Supported decode formats: WebM VP9 (video/webm; codecs=vp9) WebM VP8 (video/webm; codecs=vp8) Ogg Theora (video/ogg; codecs=theora) H.264 (video/mp4; codecs=avc1.42E01E) Supported encode formats: WebM VP9 (video/webm; codecs=vp9) WebM VP8 (video/webm; codecs=vp8)
AshleyScirra commented 5 years ago

Variadic actions are an undocumented feature. You should not be using them, and we do not support the use of undocumented features.

Variadic actions were designed exclusively for the Function object. They were never written for, tested for, or designed to work for, behaviors or non-singleglobal plugins. I'm surprised it works at all. You're in undefined territory in the engine. You should find another way to arrange your ACEs.

AshleyScirra commented 5 years ago

Oops, I just noticed variadic parameters were accidentally documented. I've removed them from the documentation, since they cannot be expected to work in the SDK. Sorry for the confusion.

Wackytoaster commented 5 years ago

2dnkhf

Ehm... well I guess that clears it up. I mean yeah they were documented, otherwise how would I ever know they even exist. :V I can live without them but they are conceptually pretty useful... No chance of adapting them or making a similar working type for behaviors/non-singleglobals?

AshleyScirra commented 5 years ago

No, we absolutely do not want to get dragged in to supporting unsupported things through the SDK. Sorry.

valerypopoff commented 5 years ago

@Wackytoaster God, this is exactly what I had a hard time debugging with my Ease plugin yesterday. My workaround was: var temp = [].concat(variadic)

@AshleyScirra There is absolutely no way we make addons that don't rely on variadic params. Please reconsider. This is not us who don't want to find another way to arrange our ACEs. We'll find it, we're developers. This is plugin users who will be left struggling to use addons that are designed in a weird way just because "oops no variadic actions anymore".

valerypopoff commented 5 years ago

15% of addons (https://www.construct.net/en/make-games/addons) use variadic params

Wackytoaster commented 5 years ago

I have to agree with valery here. I mean what alternative is there to allow users to add a variable amount of parameters? We can have them slam it all into a string that then has to be parsed properly on our side. Not nice for users, not nice for developers. I'd have to go ahead, deprecate the action and tell users: "I reworked that action, please replace it with the new one that's less convenient".

I also wouldn't want to forcefully cram variadic into it if it wasn't build for that purpose. Leave it as is and make a new alternative, call it multiparam or whatever, build for that purpose. It shouldn't be just discarded without an alternative considering its benefits.

AshleyScirra commented 5 years ago

Absolutely nothing else in Construct uses variadic action parameters other than the Function object. Everything else Construct ever does can be covered by normal parameters. Why can't you use normal parameters as well? It seems that should be sufficient. The function object is the only case when you truly need to provide a varying number of parameters.

Variadic action parameters aren't even working properly, so any addons using them are probably broken in weird ways, so they should probably avoid them anyway.

Note variadic expression parameters work differently and are supported.

We really can't afford to get dragged in to supporting these cases. We barely have the resources to do everything we need to do as it is; we can't spare the extra development time to go around fixing parts of the engine that were never meant to be used.

valerypopoff commented 5 years ago

True. Nothing else in Construct uses variadic parameters. That's because Construct doesn't do anything fancy. It's alright. It has to stay concise so you can support it. I get it. But this is the reason we're building ANY addons in the first place. The fact that Construct plugins don't use variadic params can't possibly mean that developers don't need them.

Variadic parameters are inevitable when it comes to things that: – Behave like functions – Are an addon representations of actual js-functions – Explicitly use function object as a part of their implementation

My addons wouldn't make sense without variadic parameters: – Calling js-functions in JS Plugin – Passing parameters to a callback tick-function in Ease Plugin – Rolling n-sided die in Universal Die Plugin

I understand that you can't support it right now. I can't argue with that. But we needed it, we need it, and we're going to need it anyways. That I'm ready to defend for as long as I can.

valerypopoff commented 5 years ago

Can we at least establish some kinda grey area in which we can act at our own risk? Like, for as long as you have your Function plugin, we: – Can use variadic params in Single global plugins (not behaviors) – Should remember that it's not gonna work when it comes to several instances bla-bla-bla – Don't report variadic-related issues unless it's about Function plugin – ... what else?

That way we can do our thing and you can do your thing. But this is impossible if you just delete documentation.

AshleyScirra commented 5 years ago

Are your plugins single-global? If so that should be straightforward to support, since it's similar to the Function plugin. At a minimum though our approach to developing the SDK should be a conscious and deliberate approach, and not randomly directed by accidents.

Wackytoaster commented 5 years ago

Why can't you use normal parameters as well?

Because I have a case that sort of requires it? Just because construct doesn´t have any other case where it would be required doesn´t mean that literally no other case ever exists and justifies it.

In my behavior users can create timelines with tweens. A timeline can be as short as two tweens like "tweenA, tweenB" but can also be "tweenA, wait 3 seconds, tweenB, tweenC, wait 1 second, tweenD, wait 1 second, tweenE,....." It depends on the user on how long he wants the timeline to be.

Note variadic expression parameters work differently and are supported.

So... could I theoretically feed in what I need by calling an expression? Sounds hacky.

Anyway, I can´t force you (and I don´t want to force you) I guess since you are also working on a first party tweening plugin, mine might be "obsolete" soonish and so I could say case closed.

valerypopoff commented 5 years ago

@AshleyScirra They are single-global. Except for one plugin. I think I can rebuild it so it's single-global (though it will break projects using it). But I need to understand the game rules. Is being single-global enough?

AshleyScirra commented 5 years ago

@Wackytoaster - can't you just use multiple actions which stack or sequence together?

Reopening until we figure out what to do about this.

Wackytoaster commented 5 years ago

Well, I absolutely can do that. It comes with a bit extra overhead though. Overall I think it´s an ok approach.

One thing is that it can clutter up the event sheet a bit. Right now initializing 3 timelines is 3 actions, no matter how long the timeline might be. If I assume each of them has 5 steps, that means it goes up to 15 actions. Tweens already have to be initialized individually so there would be no gain on that side, but we´re still going from 3 to 15 actions.

Timelines are referenced by an ID and can be re-initialized with new values by simply executing the initialize action again. If I sequence them together this means I first have to empty it, I can do that with an extra re-initialize/de-initialize action. This also means though that the clutter mentioned above has to be repeated.

Obviously there is potential to optimize all that with functions and whatnot but it puts the burden onto the user.

Come to think of it it might actually be pretty beneficial to separate the tween behavior from the timeline behavior, turning the timeline into a single-global, but that´s a different story. (And both these features are in development from your side so I´d wait to see how they eventually work so I don´t do unnecessary work)

AshleyScirra commented 5 years ago

I've thought about this for a while. Variadic action/condition parameters are not a strictly necessary feature. The Function plugin itself could have used a separate "Add parameter" action, which you use several times before calling a function, eliminating the need for variadic parameters. Everything else in Construct except the Function plugin follows this pattern. For example the new Drawing Canvas plugin supports drawing polygons with an arbitrary number of points; rather than use variadic parameters for the "Fill poly" action, there are "Add poly point" and "Reset poly" actions. I don't see any reason why third-party addons can't use this approach as well.

In the near future we will probably redesign how functions work in Construct: they'll probably get moved in to the System object and more deeply integrated with the event system, e.g. using real names instead of strings (similar to how layouts, object names etc. are named in the editor, and update events when renamed), using named parameters available with local variable scope in the function event, etc. Once this is done we'll want to deprecate and phase out the old Function object - after all, it was designed circa 2012, shortly after the original release of Construct 2, with the main aim of requiring the least changes to the event system in order to save time (since that has always been by far our most limited resource) whilst still being relatively convenient to use. If the Function object is phased out, there will be no more active official features that use variadic parameters. It would be great if we could then delete the code for them from our codebase: they are actually implemented as another parameter list nested within a single parameter, complicating the code in a number of places (such as being the sole reason general parameter handling code ends up being recursive/nested) and the complexity of maintaining that has been the cause of a number of bugs over the years. However backwards compatibility will mean we probably can't remove it for years. Actively supporting variadic parameters for third-party addons would prevent us ever doing that, even after years.

For the immediate purposes of not breaking existing third-party addons, the feature will be left as-is for the time being, but it should be considered deprecated, unsupported and use-at-your-own-risk. In the latest releases there will be a console warning logged if an addon uses variadic parameters. Please plan to remove any use of variadic parameters in the long term, using the "add then use" pattern instead, so we can eventually clean up our code. On that basis, closing this issue.