jbostoen / ZTStudio

A tool to read and write Zoo Tycoon 1 Graphics
GNU General Public License v3.0
9 stars 4 forks source link

Rudimentary quantization, exposed batch folder conversion, PNG <=> .pal, minor fixes #14

Closed HENDRIX-ZT2 closed 7 years ago

HENDRIX-ZT2 commented 7 years ago

Changelog: -rudimentary quantization to avoid color overflow from RGBA 32bit pngs -exposed batch folder conversion for blender -activated PNG -> palette support (works in batch mode) -simplified the palette finding code in "convert_file_PNG_to_ZT1" -added test for str to int conversion here to avoid crashes when it wants to make a graphic from a PNG palette (not sure what that code is supposed to check, tho - and why do you compare string instead of short?): (CInt(pngName) - cfg_convert_startIndex).ToString("0000") <> g.frames.Count.ToString("0000") -fixed error in clsTasks.bitmap_getDefiningRectangle(bmpDraw) that caused crashing on completely transparent images -> no longer negative size -batch conversion via console now takes a frameHold argument like: ZT Studio.exe /batchconvert:62. Frame will be held for 62 ms.

HENDRIX-ZT2 commented 7 years ago

Yeah I noticed that too. Some are just autogenerated by VS (and now with German messages - argh), and for others it wanted me to unify line endings or something. I only edited a handful of files all in all.

I thought it makes sense to specify frameHold / duration (it's not speed - that's the inverse) as the second part of the batch command, as that was previously unused. It should probably be done cleanly by storing it to the cfg and loading it from there. But I don't know how to do that, so I just went for a hacky way and passed it through the functions...

jbostoen commented 7 years ago

The line endings is what I thought it might be. I'll figure it out, you've documented all (?) changes so it shouldn't be too heard.

I agree: duration might be a better word. I'm suggesting we use /duration: (I'll implement it). It might be interesting to have it as a variable in the config file.

The general idea on the command line implementation was:

This way, the function wouldn't need another parameter to be specified.

HENDRIX-ZT2 commented 7 years ago

Ah yeah, you had two extra cases using a vbNullString instead of the second part of the argument. I thought it would make sense to remove those extra cases and put something useful into the argument.

jbostoen commented 7 years ago

Could you clarify this a bit?

-fixed error in clsTasks.bitmap_getDefiningRectangle(bmpDraw) that caused crashing on completely transparent images -> no longer negative size

I tried to create a 1 pixel transparent ZT1 graphic once, long time ago, but it crashed. So when would you have a completely transparent image?

HENDRIX-ZT2 commented 7 years ago

Sure. Say I have an animation sequence of something submerging into water. A few of the frames in that graphic have only transparent pixels. I got the error "not enough RAM" and was like "WTF is happening?". Turns out it tried to return a rectangle with negative width and heigth. So my patch just checks if the size of the rectangle is positive, and if not, changes it to be functional.

Now if a completely blank image crashes ZT1, we'd have to do something about it like add a dummy pixel or whatever, and/or notify the user. I haven't actually tested such an anim yet, as it just happened to be a a rendering error. Still it is likely that such cases occur.

jbostoen commented 7 years ago

I tried something with a hex editor back in the old days. At that point, it seemed to crash ZT1. But we could try and see if something happens (or perhaps the color palette just needs to contain a non-transparent but unused color).

In a similar situation, when creating a working Umbrella Table, I had to "hide" the guest in only one of the 4 views; and I still wanted the hand icon in ZT1 to show up. That was my use case.

I don't think there are any ZT1 graphics which contain a completely transparent frame.

jbostoen commented 7 years ago

I'm having difficulty seeing a proper difference, so I'm working on updating the code manually after all.

Command line There was a principle in the commented-out code. The idea is a user can specify an action (convert folder) and specify defaults, all in one line. But those defaults will need to be taken into account first. So you'd have the action as the last part in the line. Or if you're like me, you'd actually want it to be the first argument to clearly see what it is doing. That's why I originally just read all the settings and remembered which action to perform right after that.

Duration vs speed: I'm settling on /animationSpeed: . Why? Because it's consistent with APE. It might not be the proper terminology, but it's what Blue Fang has been using.

convert_folder_PNG_to_ZT1 The second argument was used in some cases - for a progress bar, when you batch convert in the GUI.

HENDRIX-ZT2 commented 7 years ago

But ape calls it speed and uses a speed value, ie frames per second. For zt studio, we enter the frame duration. Calling that speed is absolutely counterintuitive. Say in ape we have a speed value of 8 frames per second, that would correspond to a frame duration of 125 milliseconds, which is what we enter in ztstudio. If you want to use speed that's fine and more straightforward, but then it should really be implemented as a speed value and only converted to frame duration under the hood.

jbostoen commented 7 years ago

I'm halfway, I'm refurbishing some code/documentation as well in the process and providing some fallbacks for users who have invalid paths specified.

My current implementation for your color quantization is that it warns the user and gives the option to either use your method of getting the closest color; or simply quiting ZT Studio. It remembers this until ZT Studio is closed. You'll be able to bypass this warning by using the command line parameter /colorQuantization:1

Todo: implement your improvements for PNG palettes, look into your simplification, test if we can actually have a completely transparent frame in ZT1 after all. Options to look into:

jbostoen commented 7 years ago

In case you're sticking to your own branch after I integrated most changes, there's probably a tiny mistake in your code.

Dim exts() As String = {".png", ".gpl", ".png"}

I'm assuming the first .png was actually intended as .pal ? I like the other optimization you've done. Kudos!

HENDRIX-ZT2 commented 7 years ago

Your edit to the palette feature sounds good! I like that.

Ah yeah of course one of the pngs was meant to say pal instead.

I'm going to test an anim with some completely transparent frames ingame and see what happens.

HENDRIX-ZT2 commented 7 years ago

I have tested an animation with some fully transparent frames. It loads in perfectly in ZT Studio, APE and most importantly ZT itself. The issue with fully transparent images in your tests must have come from something else. And trust me, I know how easy it is to break things in hex... :P

fully transparent frames.zip

jbostoen commented 7 years ago

Thanks for testing. Can you repeat the test, but with only the transparent color in the .pal file? And if that crashes... with only 2 colors in the .pal file?

I'm also checking out your .PNG palette implementation. We'll need further adjustments; shortnamed PNG palettes might cause issues at the moment. Also, I think we should go for a minimal graphic - 1 px.

Also improved some other stuff along the way. Looks like ZT Studio will run a lot smoother (mainly thanks to your contributions though. I slightly changed your big optimization regarding not re-reading color palettes to improve it even a bit more!).

There was probably a reason for .ToString("0000") comparison, I'll read through the code and see if I can remember.

HENDRIX-ZT2 commented 7 years ago

Transparency: I created completely green images as input, built a palette from them using FFMPEG (first pixel green, the rest was filled with black), indexed the images and batch converted them to ZT1. The resulting PAL file had three colors, first green, then black, then green again. The result did not crash. Other anims using that PAL would display the color noise you also see in APE when a PAL can't be found. This appears to be a fallback when something refers to colors out of the palette range.

Then I removed the PNG and PAL palettes from the previous step and ran a batch conversion from ZT Studio, so it would build a fresh palette for each view. The resulting palettes all had just one green color entry. The result also worked flawlessly ingame.

So it appears we don't have to worry about completely transparent images with my patch.

We should really unify the color detection to go either via color or via index, but both just makes things super confusing. I would opt for color, and let the user specify the transparent color in the config. First pixel in image is dangerous, and first pixel in palette relies on correctly formatted input. I noticed that for some reason, FFMPEG PNG palettes don't always contain the first pixel of the image as their first color. This means you have to edit them manually before the images can be indexed and converted to ZT1.

To avoid such issues, I would suggest doing it like this: -user stores transparent color in the config, say green -if no palette is available -> use color green -if PNG/GPL palette -> convert to PAL and use green for transparency (ie. move to front regardless of where it was before) -if PAL palette -> use first index of palette

regarding the re-reading of palettes, there is still room for improvement. It should detect where the anim file is and if there is a relevant palette to use, and then just use that once and for all, and only load a new palette if a different palette is required. Before my changes, it reloaded palettes for every frame, and I think now it just reloads them for every graphic. But it should be possible to load the palette only once for all its affected files. We might have to shuffle the classes around for that.

jbostoen commented 7 years ago

Refactored a lot of code in the master branch, I'm also including your contributions and making further changes.

HENDRIX-ZT2 commented 7 years ago

Cool stuff! I'll test it out soon ;) If I ever decide to make some more changes, should I just fork your new master branch again (and refrain from breaking the line endings, hehe)?

jbostoen commented 7 years ago

We'll need to find a method where it's easier to see the differences in GitHub. I think it got messed up with line endings etc, not sure what caused this (Visual Studio? GitHub?).

Anyhow, you're credited now with contributions to the source code.

I tried to make the function to get the 'defining rectangle' a bit more efficient too.

Will look into the use of a .png palette file later (and make sure it doesn't cause any issues).

Also considering to allow conversions of color palettes on the command line, but not sure if it's of any use (yet).

As for the use of color palettes: I think we optimized it enough for now. Create or re-use per graphic is fine. "Once and for all" would make things much more complicated. What if you convert multiple animals at once, the icon also often contains very different [background] colors compared to the ones in the animations (so you'd have a degraded icon and/or animation color quantization if you'd aim for only 1 palette per animal). With some preparation, we now use palettes in the same way as Blue Fang used them for official content. Before ZT Studio, every single graphic had a separate palette. It's already a major improvement and still allows for the biggest flexibility (proper use of colors in icon vs animations).

jbostoen commented 7 years ago

-added test for str to int conversion here to avoid crashes when it wants to make a graphic from a PNG palette (not sure what that code is supposed to check, tho - and why do you compare string instead of short?): (CInt(pngName) - cfg_convert_startIndex).ToString("0000") <> g.frames.Count.ToString("0000")

I checked out that part, it's now comparing shorts. I think the .ToString("0000") part came from a copy paste in the error message. Anyhow, what it checks is whether the naming convention is valid. For a first frame, it would expect 0000.png , then 0001.png and so on (unless you specified the first file starts with 0001.png ). Basically it checks whether no files are missing and the right pattern is followed. It's documented in the code now.

I also took your note into consideration; the numeric check is a good idea; although the error could only occur in very specific circumstances. I can't spontaneously think of one at the moment. But better to avoid indeed.

I've also added a bit more flexibility to the shared palettes. If for instance you suddenly need an exceptional color palette for some reason for just one animation, you'll be able to have animals/animalid/m/walk/walk.pal if you'd want to.

Next on the list: review command line arguments. I'd like to allow as many, perhaps all settings from the config file to be overwritten. For convenience, I'm thinking of giving them the same names as in settings.cfg . Just wondering right now if I should prefix them. ( /editing.animationSpeed=125 )

HENDRIX-ZT2 commented 7 years ago

Ok, that makes sense!

"I can't spontaneously think of one at the moment. But better to avoid indeed." For my sarcosuchus, I had a global PNG palette called sarcosuc.png. It wanted to read that as a frame, and then I got the error that "osuc can not be interpeted as an integer" or something like that. Better safe than sorry

More flexible palette finding sounds great

Command line updates also sound good. Makes sense to share the names.