Skip to content

Compress CFrames using axis-angle representation#52

Merged
jackhexed merged 7 commits into
red-blox:mainfrom
Tazmondo:cframe-optimize
Jan 5, 2024
Merged

Compress CFrames using axis-angle representation#52
jackhexed merged 7 commits into
red-blox:mainfrom
Tazmondo:cframe-optimize

Conversation

@Tazmondo

@Tazmondo Tazmondo commented Jan 4, 2024

Copy link
Copy Markdown
Contributor

Effectively halves the size of CFrames from 12 f32s down to 6 f32s.

Does not deal with non-orthogonal CFrames but this is not really a big issue as discussed in the discord channel.

EDIT: For future readers, I'll explain the reasoning behind this.

  • Non orthogonal CFrames can only really be made using the specific CFrame.new constructor where you specify every element of the matrix, or with CFrame.fromMatrix. Most users will never use these constructors as they are only used when doing complex math. They can also come about from accumulated error, but correcting that is probably for the best anyway.
  • Furthermore, the use of non orthogonal CFrames are extremely limited - apparently being used mostly for skewing cameras. These use cases are rarely, if ever, going to require networking of the skewed CFrame.
  • And finally, as documented here, Add description for CFrame rotational data Data-Oriented-House/Squash#28, for the squash library, Roblox itself seems to orthonormalize CFrames before being passed over the network anyway. So, Zap isn't missing any features from Roblox's implementation anyway.

Please review harshly, I am still not super fluent with Rust so I don't know the common patterns and what good, clean, Rust code looks like.

Credits to @aidan-epperly for the idea of using axis-angle representation to compress the rotation data, as well as for giving advice on implementation and possible edge cases.

@Tazmondo Tazmondo marked this pull request as draft January 4, 2024 07:22

@sasial-dev sasial-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As discussed on Discord:

Comment thread zap/src/irgen/des.rs Outdated
@Tazmondo Tazmondo marked this pull request as ready for review January 4, 2024 15:57
@Tazmondo Tazmondo requested a review from sasial-dev January 4, 2024 16:07

@sasial-dev sasial-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Some initial comments

Comment thread zap/src/irgen/des.rs Outdated
Comment thread zap/src/output/luau/mod.rs Outdated

@sasial-dev sasial-dev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@Tazmondo

Tazmondo commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

adding docs don't merge yet

@Tazmondo Tazmondo marked this pull request as draft January 4, 2024 21:12
@Tazmondo Tazmondo marked this pull request as ready for review January 5, 2024 02:20
@jackhexed jackhexed merged commit 43ce8ca into red-blox:main Jan 5, 2024
@Tazmondo Tazmondo deleted the cframe-optimize branch January 5, 2024 18:23
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.

3 participants