AGS 3.3.0 Release Candidate

[monkey0506]

You could always abandon older versions. There's no practical reason to not upgrade once 3.3.0 is released.

[Billbis]

** WARNING **
Following functions and variables are now deprecated. If you have any of them in script you will likely receive compilation errors.
Either turn Backwards Compatibility mode in Global Settings, or consider using corresponding property from Speech class.

I feel stupid, but I have not found the corresponding backward compatibility setting.
How it is named exactly ?
Anyway, I edit my code to use the new style Speech properties, and everything run smoothly. That new aplha channel blending things is wonderful !
Congratulation and many thanks to all contributors !

[SpeechCenter]

You could always abandon older versions. There's no practical reason to not upgrade once 3.3.0 is released.

But as evident there are those who use the beta and the plugin. Someone reported a problem in the beta. Also, it would be nice to give people time to transition from 3.2.1 to 3.3.0 once it's released.
My suggestion is to keep the interface that returns scripts in the enumeration. I can live with the change that for add and remove accepts a different type since I don't call it, but I don't see why it can't enumerate the scripts as it was and force me to know the internal implementation.

[monkey0506]

In retrospect you're probably right about preserving the interface, at least for now. I pushed a new commit. If you try to iterate the Scripts class for Script objects (this now requires an explicit cast of the Scripts object to System.Collections.IEnumerable, which is compatible with the previous declaration of the Scripts class) you should get a warning about it being obsolete (actually, I don't get the warning at all), but it shouldn't prevent compilation. I also marked the Add(Script) and AddAtTop(Script) methods as obsolete, with a note to use the ScriptAndHeader variants instead.

I don't think that should actually cause any breaks in backwards compatibility. Please let me know if I've overlooked anything though, as I haven't tested it. Testing stuff is for wimps.

[Crimson Wizard]

** WARNING **
Following functions and variables are now deprecated. If you have any of them in script you will likely receive compilation errors.
Either turn Backwards Compatibility mode in Global Settings, or consider using corresponding property from Speech class.

I feel stupid, but I have not found the corresponding backward compatibility setting.

Oh, right, I forgot to mention the exact option.
It is Global Settings -> Backwards Compatibility -> Enforce object-based scripting.
This is because those changes (deprecations) disable few non-OO functions and variables.

[SpeechCenter]

The new enumerator still changes the signature.
public IEnumerator GetEnumerator() and IEnumerator IEnumerable.GetEnumerator() are different.

I created a pull request to fix that.

[Crimson Wizard]

@SpeechCenter, I have little knowledge of this area (how Scripts class is used in plugin interface), and not much free time to investigate ATM, so I hope that you will come to consensus with monkey_05_06 before this request will be merged. I'd rather wait few hours (or couple of days if required) to see his reply (and maybe tzachs too, if he has anything to say) just to be sure everyone is ok with this.

[gargin]

There appears to be a quirk in the new script grouping system. When double clicking to expand the group, a script file often opens in response. At first, I thought this was a feature as the script that opens is often the main script file for that group. I noticed that this happens even if I expand other groups (rooms list) and the mouse lands over a script file.

This is a minor inconvenience, but it does require you to continuously close script files. Just wanted to bring it to your attention.

That was actually a request, see here (in the end of the post).

While I understand that this was a feature request, it's clearly broken. Yes, if you expand a parent group the proper script file opens. The problem I am mentioning here is that if you expand ANY group using a double-click, and the mouse lands on a script file group, a script file will open. This includes just expanding the Scripts group. I have seen this behavior when expanding the Scripts group and when collapsing the Rooms group while the Scripts group is already expanded. I can't imagine this part would be intended behavior.

If necessary, I can post a video of this, but it's easy to duplicate provided you double-click to expand/collapse groups and it may only be a significant issue when multiple script groups exist (more chance of landing on one when you expand/collapse).

If for some reason this is not fixable, I would prefer to be able to toggle this double-click feature off. I tend to expand/collapse groups fairly frequently and it's a bit annoying to have to close a script windows that pop up in error. It's a minor concern, but one that should be addressed eventually.

[tzachs]

I'm getting an error when loading games or when trying to create a new game:
"The property 'LegacySpeechAnimationSpeed' could not be read. This game may require a newer version of AGS."

Please let me know if I've overlooked anything though, as I haven't tested it. Testing stuff is for wimps.

God, I hope that you were kidding...
Anyway, from my experience using classes in the Types namespace like Scripts, things that must be tested are compilation scenarios (including compiling a game which has two or more dialogs) and importing old projects.

The new enumerator still changes the signature.
public IEnumerator GetEnumerator() and IEnumerator IEnumerable.GetEnumerator() are different.

I created a pull request to fix that.

Well, if I understand correctly, you returned the previous enumeration and created a new ScriptAndHeaderEnumerator for the new enumeration of ScriptAndHeader. But didn't monkey use the new enumeration with the previous signature? You didn't change any class except Scripts, so you didn't fix those cases (unless there aren't any? If that's the case then I guess the pull request is fine if monkey has no objections).

If necessary, I can post a video of this, but it's easy to duplicate provided you double-click to expand/collapse groups and it may only be a significant issue when multiple script groups exist (more chance of landing on one when you expand/collapse).

Yes, a video would help, since I haven't been able to reproduce this issue, and frankly I don't understand how the cursor can land on another group than the one you were expanding...

[Crimson Wizard]

Please let me know if I've overlooked anything though, as I haven't tested it. Testing stuff is for wimps.

God, I hope that you were kidding...

No, he is not.

No, he is not.

No, he is not.

Okay, I am probably should not do this but fuck, I do not remember I ever was so angry in past few years.

I mean, it is no big deal when a programmer makes a mistake by overlooking. We do mistakes, we test, we bang ourselves on forehead and say "wtf I done", we fix errors.
But why pushing changes without making a SINGLE freaking test.
Why making important changes in the branch that is being prepared for release?? If that Scripts class, or whatever, was so ugly that hurt you, ou could at least change it in a parallel branch and merge later. That does not fix anything, just make better design. Wtf.

I almost believe he did this in purpose. I mean, this is probably not true, but it looks like he did.

Heck, I am spending hours and sometimes days testing changes. Maybe I should stop and say fuck it?? I'll free so many time for myself.



Damn, I am too angry, I should stop for today.

[tzachs]

I think we should give him some slack.
The understanding of the importance of testing and the sanctity of production code often comes with experience (if I remember correctly monkey is currently a hobbyist programmer, so no professional experience yet).

[monkey0506]

I have the following to say for myself:

lol

[monkey0506]

can2get copy of source that actually builds? plxkthnz

[SpeechCenter]

The new enumerator still changes the signature.
public IEnumerator GetEnumerator() and IEnumerator IEnumerable.GetEnumerator() are different.

I created a pull request to fix that.

Well, if I understand correctly, you returned the previous enumeration and created a new ScriptAndHeaderEnumerator for the new enumeration of ScriptAndHeader. But didn't monkey use the new enumeration with the previous signature? You didn't change any class except Scripts, so you didn't fix those cases (unless there aren't any? If that's the case then I guess the pull request is fine if monkey has no objections).

You understand correctly. I did test it, TDD proved itself useful on more than a single occasion, so I ignored that comment on testing, but it could have been more serious testing. By the way AGS.Native didn't compile before my change because the Script enumerator doesn't work, which is part of the compatibility problem.
Also didn't find any usages in the editor code with the enumerator, other than the AGS.Native part which wasn't changed. In all instances I found foreach goes for the AllItemsFlat which I don't think is related, but if I missed anything there then that should be fixed.

monkey_05_06 and I discussed the change over at github, he doesn't seem to like it, but with the current structure change I don't see much of a choice.
Another proposal I made was to have 2 structures, one for Scripts and the other for ScriptAndHeaders and two getters. The Scripts is built upon call and ScriptsAndHeaders returns the actual member. Scripts would then become immutable, so some compatibility is lost, but it may be ok.

I still think the change on the structure itself is least intrusive at this point, or as CW suggested, the whole change made by monkey_05_06 may be too risky if it may affect many places which weren't sufficiently tested.

[BigMc]

I have the following to say for myself:

lol

monkey, making mistakes is ok but if you refuse to learn from them and get sassy instead don't be surprised if someone decides you should better work in your own repo.

[Crimson Wizard]

I apologize for everything I said here. I am genuinely ashamed and sorry.
This outburst was in a way unexpected by me. I guess I am just worrying about these things. Maybe to much. I don't have a right to behave like this.
I think I need to take a break.
Sorry again.

[monkey0506]

I've been trying to get the native library and the engine to build, but there are some problems that needed to be addressed, not the least of which is the inclusion additional dependencies which aren't documented.

To be fair, I shouldn't have submitted major changes until I had the opportunity to fully test them.

Regarding the LegacySpeechAnimationSpeed property which I removed, I was not aware that the XML reader aborts and explodes if it encounters anything it doesn't expect. This seems very, very wrong. CW sent me a PM with a way to address that issue, but I still should have caught it myself (in testing).

As to the changes I made to the Scripts class (and inherently, the editor plugin API), I stand firm behind my work. I have made some mistakes, and I intend to learn from them to prevent causing problems for everyone else in the future. However...

* The Scripts class directly correlates to the ScriptsComponent.¹
* In the Scripts component, scripts are always added, removed, rearranged, and otherwise treated as a script and header file pair.
* The ScriptAndHeader class was implemented to refer to such a script and header file pairing.

As such:

* The Scripts class more accurately reflects a collection of ScriptAndHeader objects than a collection of Script objects.
* The primary enumerator for the Scripts class should operate on ScriptAndHeader objects, rather than Script objects.
* The Scripts class should prefer references to ScriptAndHeader objects, though reverse engineering this reference from one of the contained Script objects may be useful in production code.

And finally:

* Allowing the primary enumerator to remain as returning a Script object instead of a ScriptAndHeader object fundamentally breaks the Scripts class as a ScriptAndHeader collection.
* Obfuscation of the ScriptAndHeader enumerator to allow a non-obfuscated, broken enumerator is, if nothing else, bad form.
* The existing code which I submitted in relation to the Scripts class allows for the primary ScriptAndHeader enumerator.
* The obfuscated Script enumerator requires only a single cast of the Scripts object to System.Collections.IEnumerable, which is far less obfuscation than the proposed alternative.

The Scripts class was in a very poor state, halfway between a Script collection and a ScriptAndHeader collection. The appropriate determinable behavior is for it to operate as a ScriptAndHeader collection. As such, I stand behind my revisions, which are backwards compatible. The collection can still be enumerated by the underlying Script objects by using:

Code: csharp

foreach (Script script in (System.Collections.IEnumerable)_scripts)
{
}

By way of comparison, the proposed alternative is for the correct and primary enumerator to be obfuscated such as:

Code: csharp

foreach (ScriptAndHeader script in _scripts.ScriptAndHeaderEnumerable)
{
}

On the surface this may not look terribly bad, but it obfuscates the Scripts class as being a collection of Script objects (which it is not) and implies that additional exception must be made in order to pair the script and the header together. This is far worse style than moving forward and fixing the problem (which is that the interface was broken).

¹ There is exactly one other usage of the Scripts class in the existing code which does not relate to the ScriptsComponent. In reality, this usage would be better represented by an IList<Script> rather than using the Scripts class directly. This other usage warrants rewriting.

[SpeechCenter]

I don't think having a public property for enumeration is obfuscation. There's no requirement that the class implementing the content also implement the iterator interface.
There are various ways to implement iteration. What if you wanted a collection with forwards and reverse iterator? And what if you had a tree structure implementing both DFS and BFS iteration? Would having two properties that return the enumeration be obfuscation? I seriously doubt that. It's ok to separate implementation into multiple classes if there are multiple usages, and here there are two ways to iterate the scripts. So I would even add a second property that returns a script enumeration, showing that there are indeed two legitimate iteration scenarios and keep the interface as-is using the obsolete attribute.

Also, using your cast method would hide the obsolete warning. Who would remember that there's a possible change in the future? Plus you are still forcing code change to run, because no one would cast in the foreach statement in existing code.

Game.Scripts doesn't represent only the scripts component, it is the main way to access the scripts in the game from the plugin - that means other components use it as well, since plugins are components. The thing about backwards compatibility is allowing existing code to work. You only rely on the fact that I changed the code now to use enumeration, before that I used Count and Item, both of which are absent now, so there are additional compromises there.

But if you keep insisting that in iteration one must always pass the container and not its properties (I'm still not sure what software design principal this derives from), there's actually a third option similar to my second proposal. You could still have the Scripts class remain with exactly the same interface as before, but instead of adding/removing to its own list it adds to another instance which is the ScriptAndHeader collection. As described before, you'll have two properties in Game: Scripts and ScriptAndHeader. Scripts will continue to hold just the last header to support add/remove and have the same obsolete attribute for these methods (iteration could still be regarded as non-obsolete given that it works correctly, for example if I don't care whether the script is header or code and want the similar iteration here and when iterating room scripts)

I believe this option answers your requirement to always implement enumerable on the main class with the internal item it contains and my requirement for backwards compatibility.

[Radiant]

I was informed that on systems running a background virus scanner, each attempt to open an EXE file will trigger the virus scanner, potentially slowing down the system. This strikes me as problematic as AGS by default puts sound clips in the executable (they can be placed audio.vox, but I would expect that should be the default), as well as the sprite file and all room files (you can split off room files with a config option, but not the sprite file, and doing this generates a whole set of small resource files instead of one neat big resource file). Or perhaps more resources could simply be kept in memory (I'm thinking of the currently-playing background music, which is apparently streamed from disk); it's not like contemporary systems have a shortage of memory

Have other people encountered issues with this, and is this something that should be changed for the next build?

[Radiant]

There is talk in this thread about whether AGS should continue to use winsetup (which contains quite a number of options that are either outdated or incomprehensible to the average player). Please join the debate