Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update IAGSStream api, and use base Stream class as its direct implementation #2271

Merged

Conversation

ivan-mogilko
Copy link
Contributor

@ivan-mogilko ivan-mogilko commented Dec 20, 2023

Problem

This PR is following concerns stated in my comment to the previous IAGSStream pr here:
#2227 (comment)
summarized in a cleaner way in this PR draft:
#2256 (comment)

To put it simply, the primary goal is to make our IStream interface match IAGSStream (or other way around), and make our internal base Stream class work as a direct implementation of IAGSStream, and be able to pass Stream object to plugins, without an additional wrapper class. The purpose of this is to avoid extra wrappers over objects passed to plugins (and stream may be the first new object we expose, but there may be others in the future, such as sound player and decoder, etc). The wrappers like that make clumsy code, and they also make situation more clumsy if same object is passed back to the engine, for example, if there's a function in api that accepts such interface.

E.g. consider example:

IAGSStream* OpenFileStream(const char *script_path, int file_mode, int work_mode);
IAGSAudioDecoder *OpenAudioDecoder(IAGSStream *stream);

with this plugin may get engine's stream object from OpenFileStream and then pass it into OpenAudioDecoder function. But OpenAudioDecoder function cannot be certain that IAGSStream is engine's stream, it may be also a plugin's own implementation. So it will have to use the interface, which in turn means that the AudioDecoder class must use the stream interface.

For this purpose I also wanted to expand the stream interface, let it have bit more methods, enough to cover user's needs. Stream interface rarely needs alot of functions, so I may be getting a little over the top here, compared to laconic interface of SDL_RW, but it may look more like standard C FILE interface, or one they have in Allegro.

There may be a concern that these changes are done late in 3.6.1 development, but firstly there are no critical changes to internal classes, mostly minor changes to a few already existing methods in attempt to make them more convenient too. And secondly, I want to fix the problem with stream api before it gets "solidified" in plugins.

The PR contents may be split into two parts:

  1. Changes to IStream interface,
  2. Fixing IAGSStream in plugin api.

Changes to IStream interface.

  1. IStream has more methods now, all copied from the basic Stream class, except one is a "merger" of multiple old methods:

    • GetMode() (see explanation below),
    • GetPath(),
    • EOS(),
    • GetError() (see explanation below),
    • ReadByte(),
    • WriteByte(),
    • Flush(),
    • Close().
  2. GetMode() is a new method which merges meaning of IsValid, CanRead, CanWrite and CanSeek. It returns a combination of StreamMode flags, which directly tell what the stream can do. Currently declared are: Read, Write and Seek. Invalid streams return 0 (nothing set). This allows to use 1 method to check for all stream's abilities. The StreamMode flag set may be expanded in the future too.
    The old functions: IsValid, CanRead etc, - are still available in the base class, as non-virtual helpers. But they simply read the flags reported by GetMode() now.

  3. StreamMode itself is a merge between StreamWorkMode and FileWorkMode. It has Seek added to its constants to let report seeking ability. These flags may be both used when opening a stream and when requesting its actual mode. This means that the stream class should take care of checking the flags, and not comparing to direct values. This should not be a problem so long as you remember doing this correctly.

  4. GetError() replaced HasErrors() method. The old method was actually done incorrectly from the start. It was supposed to replicate ferror(), but the thing is that after using ferror() you also have to clear error flag if you intend to use stream further. I did not want to have extra function for that, so added a rule that GetError also resets the error flag. So it only reports error once, and not until there's another one. I think that will be fine, as streams normally should have only one user at a time.

  5. Dispose() method is added. This method deletes the stream object, and is primarily needed because of plugin api.
    The Stream base class implements this method as private, because it is not intended to be used from an actual class.
    I cannot make Close() delete the stream, because Close() may be called even from the object on stack when you want to close some stream before the variable's scope ends.

Changes to plugin api.

  1. IAGSStream is a direct match to IStream. For plugins the biggest change is that they should call Dispose() method to delete the stream instead of just Close() (they may call both, or only Dispose()).
  2. Deleted IManagedStream wrapper.
  3. IAGSEngine::OpenFileStream returns a created Stream object directly.
  4. OpenFileStream no longer saves created object in the engine's streams map. This map is only used for a) script streams, b) streams prepared for certain events, such as saving and restoring a game. Plugin can retrieve these using GetFileStreamByHandle as before.

A note on potential future changes

If #2262 stream refactor is implemented, then the IAGSStream will match IStreamBase, while Stream will work as a wrapper for IStreamBase, that can wrap both our streams and plugin streams.

Renamed HasErrors to GetError.
Require this method to clear any error flags after reading them, so to actually make this function practical.
1. Merged StreamWorkMode and FileWorkMode into StreamMode enum.
2. Introduced GetMode() method, which reports open stream's capabilities. It replaces virtual methods for IsValid, CanRead, CanWrite and CanSeek (these are kept as simpler helper methods).
3. Stream implementations should report full flag combinations, describing their capabilities, including Seeking.
Expose more methods in the interface: GetPath, EOS, GetError, ReadByte, WriteByte, Flush and Close.
IAGSStream api complies to the internal IStream.
The stream object created by the IAGSEngine::OpenFileStream call it passed to the caller directly, without being stored in the engine.
@ivan-mogilko ivan-mogilko added context: code fixing/improving existing code: refactor, optimise, tidy... context: plugin api labels Dec 20, 2023
@ivan-mogilko ivan-mogilko force-pushed the 361--iagsstream-variant2 branch from 06faa55 to 4947704 Compare December 20, 2023 01:27
@ivan-mogilko ivan-mogilko force-pushed the 361--iagsstream-variant2 branch from 4947704 to 060620c Compare December 20, 2023 01:32
@ericoporto
Copy link
Member

ericoporto commented Dec 20, 2023

streams normally should have only one user

If a plugin is reading a stream from the content of the AGS package, the plugin would not share it with the engine, right? Like, it would be still one user.

StreamMode itself is a merge between StreamWorkMode and FileWorkMode

In the filestream, there is an internal _openMode, is it necessary?

@ivan-mogilko
Copy link
Contributor Author

If a plugin is reading a stream from the content of the AGS package, the plugin would not share it with the engine, right? Like, it would be still one user.

By "user" I meant a code that calls the object's functions at this time. Even if engine has stream ownership (stores and deletes it), it may still give it out to plugin to use, how it is done during game saving and loading. The point is that while plugin reads or writes into it, the engine does not, until the callback finishes.

In regards to "ownership", the streams created with OpenFileStream are now owned strictly by plugin, engine returns and forgets about them. The streams given out with GetFileStreamByHandle are NOT owned by plugin, plugin is supposed to use them for its needs during some callback and then leave be.
Thinking of this, maybe I should add a flag telling whether a stream by handle is strictly for scripts of plugins, to avoid plugins randomly get script streams.

@ivan-mogilko
Copy link
Contributor Author

In the filestream, there is an internal _openMode, is it necessary?

I had a false memory that there's a method which returns it, but apparently no. I may add this, but in any case, this variable is there only for debugging purposes.

@ericoporto
Copy link
Member

Overall this looks alright, I think this should be merged before the next beta release, to get it to the plugin interface.

@ivan-mogilko
Copy link
Contributor Author

For the record, these are fixes to spritevideo plugin after these changes:
ivan-mogilko/ags-spritevideo@3401afc

This prevents numerous warnings in stricter compilers.
@ivan-mogilko ivan-mogilko merged commit aafc185 into adventuregamestudio:master Dec 20, 2023
20 checks passed
@ivan-mogilko ivan-mogilko deleted the 361--iagsstream-variant2 branch December 20, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
context: code fixing/improving existing code: refactor, optimise, tidy... context: plugin api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants