Skip to content

Adding a modification framework to DiveBlockData #224

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

angela28chen
Copy link
Collaborator

@angela28chen angela28chen commented May 30, 2025

Refactoring DiveBlockData and adding functionality to store and apply block-level modifications.

  • The location of a modification is described with primary_index and secondary_index, describing which original block it's anchored to and how it is positioned in relation to the anchor block (negative for immediately before, 0 for overwriting, positive for immediately afterwards)
  • Original block metadata and modification block metadata are stored separately so that modifications can be manipulated easily without affecting the original data.
  • BlockVisitor is defined using the Visitor design pattern to hold handles to open files (original and new gfxr files) and write block-by-block in the new order

@angela28chen
Copy link
Collaborator Author

angela28chen commented Jun 2, 2025

I have tested this code manually on windows and it works, but it's a pain to add new flags to divecli so I want to review this portion first. I will hold off on submitting this until we decide the following and I update this PR:

  1. Where to put dive-specific GFXR code (note: unit tests)
  2. Should we continue extending the functionality of the current divecli, or refactor it to use absl::flag and greatly reduce the friction of adding new functionality?

@angela28chen angela28chen changed the title [WIP] Adding modification framework to DiveBlockData Adding a modification framework to DiveBlockData Jun 2, 2025
@angela28chen angela28chen marked this pull request as ready for review June 2, 2025 21:25
};

// Abstract class representing a binary block encoded in .gfxr format
class DiveSingleBlock
Copy link
Collaborator

Choose a reason for hiding this comment

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

just curious, it is named as SingleBlock because there will be cases for MultiBlock in the future?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't considered multi block units. This one is just named single block to distinguish it from DiveBlockData (which is meant to hold a bunch of single blocks. If you have different ideas for naming, any suggestions are welcome.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe simply IDiveBlock?


// Representing a gfxr-encoded block in a buffer
// If blob_ptr is undefined, that is interpreted as an empty block
class DiveSingleGfxrBlockInBuffer : public DiveSingleBlock
Copy link
Collaborator

@wangra-google wangra-google Jun 3, 2025

Choose a reason for hiding this comment

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

just to make sure i understand correctly, all modifications will be in DiveSingleGfxrBlockInBuffer? if that is the case, would it be better to call this DiveModificationBlock? (and the other one as DiveOriginalBlock)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I think that might be easier naming to understand so I will change it. I picked names to describe where the actual encoded bytes of the block exist, but I think original/modified is a bit easier to read.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks

@angela28chen
Copy link
Collaborator Author

Note: blocked by #234 because I want to hook this feature up to the CLI for testing

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