Skip to content

support configurable output data writers in recorder#17

Open
ahmed-shariff wants to merge 4 commits intoliris-xr:masterfrom
ovi-lab:master
Open

support configurable output data writers in recorder#17
ahmed-shariff wants to merge 4 commits intoliris-xr:masterfrom
ovi-lab:master

Conversation

@ahmed-shariff
Copy link
Copy Markdown

This PR refactors the recorder infrastructure to support configurable output data writers. The IDataWriter interface is now generic on a new IDataWriterInfo, allowing data writers to receive configuration via info objects (FileDataWriterInfo, NetworkDataWriterInfo) when instantiated. StartRecordingInternal and StartRecording accept an array of IDataWriterInfo to flexibly control recording destinations and configurations at runtime. This enables future extensibility, such as adding new writer types or enabling user selection of outputs. Default behavior remains writing to a file when no outputs are specified. Added info classes for file and network writers to contain configuration like file paths and network endpoints.

An alternative to having IDataWriterInfo is to directly pass the IDataWriter instances, and pass the record instance through an Initialize method.

This is primarily motivated by wanting to control when data recording starts and stops and as an extension also specify where and how the data would be recorded.

If necessary, the implementations allows adding support to configuring the writers from settings as well.

Let me know what you think about this.

Refactored the recorder infrastructure to support configurable output
data writers. The IDataWriter interface is now generic on a new
IDataWriterInfo, allowing data writers to receive configuration via
info objects (FileDataWriterInfo, NetworkDataWriterInfo) when
instantiated. StartRecordingInternal and StartRecording accept an
array of IDataWriterInfo to flexibly control recording destinations
and configurations at runtime. This enables future extensibility, such
as adding new writer types or enabling user selection of
outputs. Default behavior remains writing to a file when no outputs
are specified. Added info classes for file and network writers to
contain configuration like file paths and network endpoints.
This also makes GenerateFilePath public to allow setting file names
externaly.
Comment on lines +89 to +94
outputsList.Add(info switch
{
FileDataWriterInfo => (IDataWriter<IDataWriterInfo>)new FileDataWriter(record, (FileDataWriterInfo)info),
NetworkDataWriterInfo => (IDataWriter<IDataWriterInfo>)new NetworkDataWriter(record, (NetworkDataWriterInfo)info),
_ => throw new InvalidOperationException()
});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to directly give a list of IDataWriter[] as input to StartRecordingInternal, avoiding this switch to be changed for every new IDataWriter type. Additionnally, this would get rid of the DataWriterInfo class as the DataWriter could be constructed directly outside of the recorder and it wouldn't be the responsibility of the recorder to construct the writers.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would agree that it would be a better approach. However, the record object needs to be injected. Let me try restructuring that to see what it could look like.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored to allow for injecting the recorder. Let me know what you think.

Replaces the use of IDataWriterInfo-based writer construction with
direct use of initialized IDataWriter instances throughout the
Recorder and related classes. FileDataWriter and NetworkDataWriter now
store configuration parameters directly and provide an Initialize
method for setup at runtime, rather than being constructed via
metadata wrapper classes. The Info and Info-based writer classes and
interfaces are removed in favor of a simpler, explicit initialization
paradigm.
When the recorder restarts, _lastFixedUpdateTime isn’t reset, so
RunFixedUpdate tries to process every frame recorded during the
downtime and causes a spike. To prevent this, we reset
_lastFixedUpdateTime to the current time on restart, ensuring only new
frames are processed.
@cjaverliat cjaverliat marked this pull request as draft June 10, 2025 19:32
@cjaverliat cjaverliat self-assigned this Jun 10, 2025
@cjaverliat cjaverliat marked this pull request as ready for review June 10, 2025 19:34
@cjaverliat
Copy link
Copy Markdown
Contributor

Thanks for taking the time to work on this :)
I like the direction toward more flexible and configurable data writers.

A few thoughts after going through the changes:

  • Regarding the idea of using an Initialize method on IDataWriter: that approach feels a bit non-standard for writer interfaces. It might be cleaner to pass the necessary metadata or config (like Record info) directly when creating the writer. That said, I realize doing so might require some refactoring of existing classes.

  • I think the current architecture, as we designed it, with Record, Recorder, RecorderContext, and DataDispatcher might be unecessarily complex, and that’s making it harder to support multiple data writers cleanly. For example, Record currently holds the serialized data chunks in memory, which are then pulled by the DataDispatcher to be written to the file/network data writers. The Recorder methods like Recorder.RecordTimelessSample, Recorder.RecordMarker, etc simply deflect the calls to _context.CurrentRecord.RecordTimelessSample, _context.CurrentRecord.RecordMarker and so on. It might be worth considering merging some of these classes. I think a simpler structure, where the Recorder contains its recorder modules (currently stored in RecorderContext), a structure to hold the data buffers, and handles recording logic directly, could simplify things and make multiple writer support cleaner.

  • Specifically, the Record class might not be needed anymore. If we move the buffer storage into Recorder, then methods like RecordTimelessSample or RecordMarker could just write directly to those buffers, without routing through _context.CurrentRecord.Record....

Of course, this implies bigger change to the PR, I would be happy to discuss further or help break it down if we want to pursue this refactor and simplification.

@ahmed-shariff
Copy link
Copy Markdown
Author

Regarding the idea of using an Initialize method on IDataWriter: that approach feels a bit non-standard for writer interfaces.

I agree, I find dependency injection to nearly always gets messy.

It might be cleaner to pass the necessary metadata or config (like Record info) directly when creating the writer.

I see your point about Recorder, Record, RecordContext and DataDispatcher. From a cursory glance, it should be possible to consolidate these classes into one? However, in the context of this PR, the record is being passed only to get the metadata. For now I'll try to restructure this to use a metadata setter.

@ahmed-shariff ahmed-shariff force-pushed the master branch 3 times, most recently from 723b2ea to aef23f7 Compare June 12, 2025 03:28
Refactor the Recorder system to initialize IDataWriter outputs using
the new SetMetaData method instead of Initialize. FileDataWriter now
requires file paths on construction and accepts metadata via
SetMetaData. NetworkDataWriter implements a no-op SetMetaData to
satisfy the interface. This change improves separation of
responsibilities by decoupling metadata setup from generic Initialize
calls.
@ahmed-shariff
Copy link
Copy Markdown
Author

My understanding is that RecordMetaData itself cannot be made available to the writer objects when they are created, as the metadata is set up when the recording starts. Hence, only the metadata is injected. This way, all other setups during the construction of the writer are left as is. Let me know if this makes more sense for now.

I can take a stab at the refactor you mentioned above in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants