New game builder issues (since 3.4.0.2)

Started by Crimson Wizard, Tue 18/11/2014 10:46:52

Previous topic - Next topic

Crimson Wizard

I experience this exception with the new compiler when building a game based on "9-verb MI" template:

Code: csharp

public void WriteViews(IViewFolder folder, Game game)
{
    if (writer == null)
    {
        throw new CompileError("Could not write views: Invalid stream (NULL)");
    }
    foreach (View view in views)
    {
        short numLoops = (short)view.Loops.Count; <--------- HERE
        writer.Write(numLoops);


Exception is: System.NullReferenceException; cause: 'view' object is null.
It appears that 'views' collection may contain null-pointers. In the case of "9-verb MI", the first non-null View is under #9.

It also appears that 'game.ViewCount' returns the number of the highest View ID, not the real number of views, therefore 'views' collection may be very large, yet contain only few actual View objects.

UPD.
The way this was done in AGS.Native:
Code: cpp

int viewCount = AGS::Types::FolderHelper::CountViews(AGS::Types::FolderHelper::GetRootViewFolder(game));

Also views were converted using some kind of helper:
Code: cpp

AGS::Types::FolderHelper::ViewFolderProcessing ^del = 
    gcnew AGS::Types::FolderHelper::ViewFolderProcessing(ConvertViewsToDTAFormat);
AGS::Types::FolderHelper::ForEachViewFolder(folder, game, del);

for each (View ^view in folder->Views)
{
<...>


I think this is because views always keep their ID, even if some were deleted, unlike Characters, etc.

Crimson Wizard

#1
More importantly, "Use legacy compiler" preference does not do what it is expected to. I believe this is partially my fault, I overlooked this.

It was supposed to call old functions that write data, instead it calls old functions that just combine data in a big file, but the data itself is still written with the new DataWriter; therefore it is impossible to fallback to old method if any errors occur.

monkey0506

#2
When I constructed the list of views for the ViewsWriter class I forgot that views preserve their ID if they are deleted. All it needs is a check if the view is null:

Code: csharp
            public void WriteViews(IViewFolder folder, Game game)
            {
                if (writer == null)
                {
                    throw new CompileError("Could not write views: Invalid stream (NULL)");
                }
                foreach (View view in views)
                {
                    if (view == null) continue; // views are not always sequential, so we may have some null entries
                    short numLoops = (short)view.Loops.Count;
                    /* ... */
                }
            }


In BuildTargetDataFile I forgot to check if for the legacy compiler flag (here, NativeProxy.CreateGameEXE checks it properly).

Code: csharp
        public override bool Build(CompileMessages errors, bool forceRebuild)
        {
            if (!base.Build(errors, forceRebuild)) return false;
            Factory.AGSEditor.SetMODMusicFlag();
            DeleteAnyExistingSplitResourceFiles();
            if (Factory.AGSEditor.Preferences.UseLegacyCompiler) Factory.NativeProxy.CompileGameToDTAFile(Factory.AGSEditor.CurrentGame, AGSEditor.COMPILED_DTA_FILE_NAME);
            else DataFileWriter.SaveThisGameToFile(AGSEditor.COMPILED_DTA_FILE_NAME, Factory.AGSEditor.CurrentGame);
            /* ... */
        }


I can submit commits for these shortly.

Edit: Fixed with commits 029461e and 8117631.

Edit (again): This also points out that the WriteViews function is taking an IViewFolder as a parameter but it always writes the root folder. This parameter was an artifact of a previous version, but there's no reason that it couldn't be implemented (but that's for another time).

Crimson Wizard

With the recent changes:

1. The compilation of 9-verb template produces corrupted data somewhere between lipsync and global messages array. Cannot tell whats wrong right away.

2. With the legacy compiler option enabled Editor produces "game_name.exe" file in the Compiled folder and another "game_name.exe" file in Windows folder.
The first cannot be run, because it is not a valid executable, the second can be run, but tells that ther's no game data found.

Gurok

Translation files (*.tra) aren't copied to the Windows platform directory inside Compiled. The resulting .exe can't find any translations. :/
[img]http://7d4iqnx.gif;rWRLUuw.gi

Monsieur OUXX

I didn't realize there was a new compiler. Where can I learn more about that?
 

Crimson Wizard

Quote from: Monsieur OUXX on Fri 28/11/2014 21:44:11
I didn't realize there was a new compiler. Where can I learn more about that?
http://www.adventuregamestudio.co.uk/forums/index.php?topic=51050.msg636500056#msg636500056

It's a game compiler, not script compiler. It is made in C# as opposed to C++. Its just another way to do same thing, which is also a part of building a game for other platforms. It is not supposed to change anything in how game works (bugs do, but so are bugs).

Crimson Wizard

Quote from: Crimson Wizard on Fri 21/11/2014 13:55:47
1. The compilation of 9-verb template produces corrupted data somewhere between lipsync and global messages array. Cannot tell whats wrong right away.
I found that the problem is still in the Views. The original compiler still saves the number of views equal to the max view ID. Its just that it specifies loop count = 0 for every non-existing view.

Gurok

I've been meaning to say this for a while. I think this might be better termed as a new linker or maybe a new builder, not a new compiler. (http://en.wikipedia.org/wiki/Linker_%28computing%29)
[img]http://7d4iqnx.gif;rWRLUuw.gi

Crimson Wizard

It is data writer. I am not sure where the "compiler" came from.

Gurok

Yeah, it might be good to clarify when it comes time to do the release notes. I think some people are already confused and I know there are a lot of people clamouring for a non-native (.NET based) compiler.
[img]http://7d4iqnx.gif;rWRLUuw.gi

Crimson Wizard

#11
Quote from: Crimson Wizard on Thu 04/12/2014 01:35:20
Quote from: Crimson Wizard on Fri 21/11/2014 13:55:47
1. The compilation of 9-verb template produces corrupted data somewhere between lipsync and global messages array. Cannot tell whats wrong right away.
I found that the problem is still in the Views. The original compiler still saves the number of views equal to the max view ID. Its just that it specifies loop count = 0 for every non-existing view.

Ok, I think I fixed this one now. At least 9-verb MI template now builds and runs properly.



UPD
Quote from: Crimson Wizard on Fri 21/11/2014 13:55:47
2. With the legacy compiler option enabled Editor produces "game_name.exe" file in the Compiled folder and another "game_name.exe" file in Windows folder.
The first cannot be run, because it is not a valid executable, the second can be run, but tells that ther's no game data found.

Actually, the Editor simply does not copy engine binary to Compiled folder anymore (where it makes game with legacy compiler), hence the game is attached to empty file, or whichever file existed there previously.

Gurok

Hey CW, I have a pull request to fix .vox and .tra file copying. I think it might be the cause of the 3.4.0.2 error, "Cannot create hard link! Source file does not exist."
[img]http://7d4iqnx.gif;rWRLUuw.gi

Calin Leafshade

If a dialog (or indeed other items i imagine) exceed the maximum allowed length then the new data writer throws "Can't resolve import dMyDialogWithALongName" but the old compiler gives the correct error message about the length.

Crimson Wizard

#14
Quote from: Gurok on Sat 13/12/2014 21:14:07
Hey CW, I have a pull request to fix .vox and .tra file copying. I think it might be the cause of the 3.4.0.2 error, "Cannot create hard link! Source file does not exist."
Well, I wrote this on Github, but I can tell here too, that I really do not like the inconsistency between Linux and Windows builder classes. Linux builder takes every file from Compiled folder, while Windows one requires to have their names hard-coded. They should work same way. I'd say that probably even builder interface should be changed, because the copying/link of files is a universal operation. I mean that the list of game files should probably be provided by external user.

monkey0506

#15
Sorry about falling off the radar there. Finals at school kind of took precedence.

I'm trying to figure out what the current state of affairs is now though. I know there are some bugs in the build code right now, and I am more than happy to help track them down. It seems Gurok has already caught on to the fact that AGS.Editor.Utilities.GetDirectoryFileList isn't returning the values I thought it was, which is leading to some of the incorrect paths in the hard-linking code (however, I would suggest against using hard-coded directory separators, and instead use Path.DirectorySeparatorChar Path.GetFileName).

Quote from: Crimson Wizard on Thu 04/12/2014 10:51:56Actually, the Editor simply does not copy engine binary to Compiled folder anymore (where it makes game with legacy compiler), hence the game is attached to empty file, or whichever file existed there previously.

I'll take a look at this. It might make sense to delete the Compiled folder when changing this setting, and of course the legacy code should still be copying the engine EXE as it was previously.

Quote from: Crimson Wizard on Mon 15/12/2014 17:29:24I really do not like the inconsistency between Linux and Windows builder classes. Linux builder takes every file from Compiled folder, while Windows one requires to have their names hard-coded. They should work same way. I'd say that probably even builder interface should be changed, because the copying/link of files is a universal operation.

I'm really not sure what you mean by this. Neither the BuildTargetDataFile class nor the BuildTargetWindows class reference any hard-coded paths or file names. As for copying/linking being a "universal" process, I suppose that I could add to the BuildTargetBase class to force the build targets to provide file name, file source path, file destination path, and destination mode (link or copy) for all required build files. Currently both Windows and Linux are parsing the Compiled directory's files directly, so it is only necessary to reference exact file names for platform-specific handling (which is currently only done by the Linux class as Windows doesn't require any platform-specific files). Assuming that the build targets could default some of these parameters, then I could perhaps see some merit to this.

Edit: I do see where the Windows build target is copying/linking the exact files referenced by the previously existing code, whereas Linux is just copying/linking any file from the Compiled directory. This is an inconsistency. Personally I feel that linking any arbitrary files the user has placed in the Compiled directory is a good feature for the end-user. This would allow game devs to put a single README file in the Compiled directory and have it perpetuated to the various platform folders (on build).

Quote from: Crimson Wizard on Mon 15/12/2014 17:29:24I mean that the list of game files should probably be provided by external user.

The entire purpose of having platform-specific classes is to provide the list of platform-specific files, and the platform-specific methods, to allow building that platform. I'm not sure how much further abstracted it could get than described above to where the build target classes tell the base class what files they need, where they need them, and whether to link or copy them.

Quote from: Calin Leafshade on Sun 14/12/2014 16:09:32If a dialog (or indeed other items i imagine) exceed the maximum allowed length then the new data writer throws "Can't resolve import dMyDialogWithALongName" but the old compiler gives the correct error message about the length.

Yes, it's probably worth running back through for error messages that weren't implemented. I was pulling source from dozens of different places, utility classes, subclasses, direct memory access, and so forth, so some of the error messages fell by the wayside as I was sorting out what was necessary to getting the data file in the correct format. These error messages could be nullified by removal of limits, but I should make sure that they respect any limits that still exist in the engine (where the compiled data file is read back in).

monkey0506

Please pardon the double-post, but I think I've just about honed in on the issue with the legacy compile option not working. Foolishly, I wasn't telling the other build targets to not build if the legacy compiler was selected. The BuildTargetDataFile class will appropriately defer to that setting, and there is some code that was moved into BuildTargetWindows that still needs to be called, but the rest should be ignored from that point. I have a nearly-working build, and by nearly-working I mean that it almost works except that it doesn't. ;) The size of the generated EXE looks correct, and it's properly updating the icon which is a good sign, but it's not finding the data file embedded in the EXE (possibly I need to write the offset manually, or something). I will look into this, but for now I am falling asleep, so.

Crimson Wizard

#17
An up-to-date situation:

1. The Compiled folder should be cleaned up of anything that belongs to game compilation output as soon as building starts, if the compiler type was previously changed (legacy <-> new).
This should not delete any extra files that were put there by user. If any folders get empty after files were deleted, these folders should be removed as well.
I know we may remove legacy builder soon after we finish fixing new one, but this will make life easier for now.

2. Important changes to acsetup.cfg are not copied to final build folders.
* Create a default game (320x200) and build it.
The config file in every final folder (Windows, Linux) will be identical to the one in base folder.
* Change resolution to 320x240 and build again.
The config file in the Compiled folder will have new resolution, but the config file inside Windows or Linux folders will not. (NOTE: this does not affect game itself, but how setup will work).

Expected behavior: there is a number of options that serve as hints to setup program. These hints must be updated in the config files every time.
At the moment they include:
+ game size
+ game color depth



3. When the setup is launched from the editor, it modifies the config in Compiled/Windows folder. At the same time _Debug folder has config file copied from Compiled folder, not Windows folder (or maybe its generated from scratch, idk).

Expected behavior: Debugging is done on the version of user OS (currently it may only be Windows), therefore _Debug folder must have contents copied from related folder (Windows).



E: 4. I kinda think that we should move the base output to subfolder too (e.g. Compiled/Base). This will make it easier for user to understand which files should be copied if he wants "pure" game data without executable.


EDIT2: I haven't yet learnt everything about how Build targets work, but I have the impression that some things there are unnecessarily complicated, such as composing output paths. I believe that build target should not need to define which path to build output in, but rather receive one from caller.

Crimson Wizard

I fixed config problems (points 2 and 3) as the only important at the moment. Other points would require a bit of refactoring.

monkey0506

Quote from: Crimson Wizard on Sat 18/07/2015 16:17:11EDIT2: I haven't yet learnt everything about how Build targets work, but I have the impression that some things there are unnecessarily complicated, such as composing output paths. I believe that build target should not need to define which path to build output in, but rather receive one from caller.

The idea behind this was that the caller shouldn't have to know anything about the build target in order to build it. Perhaps that was contrived or misguided, but my primary concern really was getting everything to work properly by that stage.

Quote from: Crimson Wizard on Sat 18/07/2015 16:17:112. Important changes to acsetup.cfg are not copied to final build folders.

The original rationale behind this was that the developer may wish to have separate configuration options based on the platform, so overwriting the platform configuration was intentionally avoided. Perhaps important changes (resolution, color depth) could be persisted without overwriting the entire config file?

SMF spam blocked by CleanTalk