TypesettingTools / SubInspector

Low-level subtitle inspection library.
MIT License
5 stars 3 forks source link

Massive Library Restructuring. #10

Closed torque closed 10 years ago

torque commented 10 years ago

Okay, so after claiming I didn't have time to work on this, I had a discussion with @line0, and got fixated on a stupid idea that I decided to implement. So I implemented it. And now I'd like to talk about it.

disclaimer: I am not necessarily suggesting merging this pull request wholesale into master as it contains a lot of changes, some of which may be controversial or bad.

The idea

Initially, the plan was to hack in bounding box calculation which, while somewhat more rigorous than simply checking for a dirty pixel, is also far more useful.

Highlights

Events are no longer stored in the library

After that long discussion about the best way to store events in #7 and talking to @line0 about it, I realized there wasn't even really a good reason to actually store events.

Initially implemented in 18b8f68, assi_setScript takes two strings as input (one optionally NULL) and generates a minimal script with it. Note that the script header is still stored on library init, as it is unlikely to change in between runs.

Error strings can now be accessed

const char* assi_getErrorString( ASSI_State* ) was added in 3d84636, allowing users to actually access the error string. Additionally, a few internal errors actually set the error string properly.

A well-commented example script has been added

OptimASS aside, the easiest way to test the API and document it simultaneously was to write an example for how it is intended to be used. Note that the example is Aegisub-specific, but should be usable by anyone.

Font management

As of e696a84, it is possible for the user to specify a directory that fontconfig should search for fonts. This provides a convenient alternative to fontconfig taking time to cache all of the system's fonts.

Uses libass's caching mechanism

c863c8f implements libass's reporting of sequential lines that are identical. This allows the bounding box to be copied (or moved) rather than having to check for it all over again.

Issues

Lots of code duplication

It seems I am incapable of both implementation and design simultaneously. Or perhaps I am just incapable of design. The loop that does bounds checking is present three times in the source, and the code for checking moved events vs checking new events is very similar.

Doesn't take some short cuts

Based on observation, it seems that if the width (as given by ASS_Image->w) is not padded to the nearest mod 16 integer (which only seems to happen if a clip is present) then the reported width is the exact dirty rect width. The same holds true for height.

However it is possible that there is some weird case that taking this shortcut would not work on.

Youka commented 10 years ago

I wouldn't have put styles & events together and pass the header only to initialization, it's too much designed for your Aegisub macro only.

The initialization should just create the object with external circumstances, script parts should be editable to use other scripts or properties/sections for the same frame size & fonts cache.

Additionally i keep with the point passing lengths isn't required because ASS is a text format, no null-character to expect in input except at the end.

torque commented 10 years ago

Looking at it, passing the header to initialization was a bad idea. I htink having a separate addHeader function is more appropriate. This allows the user to change the header without reinitializing the entire library.

I finally agree that passing the lengths is unnecessary. Will push a couple of commits today or tomorrow dealing with these changes.

Youka commented 10 years ago

Rather a setter with upcoming getter functions, making an inspector object capable to be used for multiple scripts / headers / styles / events, sharing the same target video & fonts collection.

torque commented 10 years ago

I'm not sure I understand what the point of allowing multiple scripts would be. Perhaps it's because I'm locked in on a single implementation goal, but I can't think of a situation where having multiple scripts/headers, etc, would be desirable.

And if I can't think of a scenario in which they'd be useful or desirable, I won't be able to implement them in a useful fashion. Perhaps you could provide an example of why being able to have multiple scripts would be convenient?

torque commented 10 years ago

I'm going to merge this in 2 days if there are no major grievances. I'd like to make another release from the official repo.

Any redesign can, of course, still happen. But I'd rather have something that works for now.