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

🚧Refactor GlyphMetrics to readonly record #803

Closed
9 tasks done
CalvinWilkinson opened this issue Nov 12, 2023 · 4 comments · Fixed by #820
Closed
9 tasks done

🚧Refactor GlyphMetrics to readonly record #803

CalvinWilkinson opened this issue Nov 12, 2023 · 4 comments · Fixed by #820
Assignees
Labels
🧨breaking-changes Feature/changes contains breaking changes medium-priority Medium Priority 🏎️performance Performance improvements preview Done while in preview tech-debt Code refactoring or cleanup / tech debt reduction

Comments

@CalvinWilkinson
Copy link
Member

CalvinWilkinson commented Nov 12, 2023

Complete The Item Below

  • I have updated the title without removing the 🚧 emoji.

Description

Refactor the GlyphMetrics struct to a readonly record struct. This will provide performance benefits as well as make the struct data immutable. When refactoring the code, use the with keyword.

This struct holds data required for creating font atlas textures when loading fonts, which dictates how the text will be rendered with a particular font.

Also, refactor the properties to public init properties. This will provide the ability to set the initial values of the properties as well as keep the structure immutable.

Make sure that the XML docs for the properties have been updated.
Example:

/// <summary>
- /// Gets or sets the value.
+ /// Gets the value.
/// </summary>
- public uint MyProp { get; set; };
+ public uint MyProp { get; init; }

Note
Please use the full prop definitions as shown above. Do not use any primary constructors.
Further changes are coming down the road related to primary constructors and some research needs to be done first.

Warning
Be careful with refactoring code related to this struct. The impact could entirely break the font creation process and in turn how text is rendered. This code is especially sensitive because unsafe code is used to bit shift values on this struct based on 32bit vs 64bit processor architectures. This sensitive code is located in the FontService class.

Warning
This issue will introduce https://github.com/KinsonDigital/Velaptor/labels/%F0%9F%A7%A8breaking%20changes due to the GlyphMetrics struct being part of the public API.

Things To Consider:

  • The IEquatable<T> is not required for record's. This is because the Equals() method that the interface provides is built into records.
  • Methods like the Equals() override bool Equals() and GetHashCode() are built-into records

Delete the GlyphMetricsTests file. The struct is so simple combined with the removal of the Equals() methods, that there is nothing to test anymore.

Acceptance Criteria

  • The GlyphMetrics structure has been refactored to a readonly record struct.
  • Related code that uses the struct has been refactored
  • The GlyphMetricsTests file has been deleted.
  • XML docs updated where applicable

ToDo Items

  • Change type labels added to this issue. Refer to the Change Type Labels section below.
  • Priority label added to this issue. Refer to the Priority Type Labels section below.
  • Issue linked to the correct milestone (if applicable).

Issue Dependencies

No response

Related Work

No response

Additional Information:

Change Type Labels

Change Type Label
Bug Fixes 🐛bug
Breaking Changes 🧨breaking changes
New Feature ✨new feature
CICD Changes ♻️cicd
Config Changes ⚙️config
Performance Improvements 🏎️performance
Code Doc Changes 🗒️documentation/code
Product Doc Changes 📝documentation/product

Priority Type Labels

Priority Type Label
Low Priority low priority
Medium Priority medium priority
High Priority high priority

Code of Conduct

  • I agree to follow this project's Code of Conduct.
@github-project-automation github-project-automation bot moved this to ⚪Not Set in KD-Team Nov 12, 2023
@github-actions github-actions bot added the ⚕️NEEDS-TRIAGE Issue needs to be looked at and processed by a team member label Nov 12, 2023
@CalvinWilkinson CalvinWilkinson added tech-debt Code refactoring or cleanup / tech debt reduction medium-priority Medium Priority preview Done while in preview 🏎️performance Performance improvements and removed ⚕️NEEDS-TRIAGE Issue needs to be looked at and processed by a team member labels Nov 12, 2023
@CalvinWilkinson CalvinWilkinson added this to the v1.0.0-preview.31 milestone Nov 12, 2023
@CalvinWilkinson CalvinWilkinson added the 🧨breaking-changes Feature/changes contains breaking changes label Nov 12, 2023
@map-b
Copy link
Contributor

map-b commented Nov 23, 2023

Starting work on this.

map-b added a commit to map-b/Velaptor that referenced this issue Nov 23, 2023
@CalvinWilkinson CalvinWilkinson moved this from ⚪Not Set to 🏗️In Development in KD-Team Nov 24, 2023
@CalvinWilkinson
Copy link
Member Author

Starting work on this.

@map-b I have assigned the issue to you.

Please note, I added a note in the description above stating to make sure to keep the prop definitions in the struct and to not use primary constructors.

Cheers!!

@map-b
Copy link
Contributor

map-b commented Nov 24, 2023

Of course! I didn't see it helpful in this case yesterday. I don't know if keeping the definition of the constructor although It isn't used?

@CalvinWilkinson
Copy link
Member Author

Of course! I didn't see it helpful in this case yesterday. I don't know if keeping the definition of the constructor although It isn't used?

Thanks. I appreciate that!!

As of right now, it looks like I will be avoiding primary constructors when it comes to dependency injection, and then data holding types will probably move to primary constructors. But I need to think about what problems might arise, performance, .editorconfig setup for analyzers, etc.

I am super excited to move to dotnet 8 though!!

If you are interested in following (watching) issue #810 as I keep adding to it and doing research, you are welcome to.

If you are interested in knowing the state of the entire organization and all of it's projects, you can view the org project here

CalvinWilkinson pushed a commit that referenced this issue Nov 27, 2023
* Start work for issue #803

* Refactor: GlyphMetrics struct to readonly record struct.

* Fix "GameHelpers-> ApplySize" return value override actual values.

* Refactor: Fix tests using "with" for "readonly record struct" initializations.

* Clean up

* Only use the "with" keyword when it is needed (avoid unnecessary copy).

* Remove old struct custom tests.
@github-project-automation github-project-automation bot moved this from 🏗️In Development to ✅Done in KD-Team Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧨breaking-changes Feature/changes contains breaking changes medium-priority Medium Priority 🏎️performance Performance improvements preview Done while in preview tech-debt Code refactoring or cleanup / tech debt reduction
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants