Skip to content

Conversation

@is-this-c
Copy link
Contributor

No description provided.

Copy link
Owner

@rafalh rafalh left a comment

Choose a reason for hiding this comment

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

Please stop mixing formatting and code style changes with actual behavior changes in one commit. This makes reviewing more time consuming. Splitting changes into multiple logically separate commits with messages describing what changed and optionally why, is the best way to mix such changes. You can leave it mixed in this PR, but please don't do it in the future.


// Fix `rf::gr::text_2d_mode`.
AsmWriter{0x0050BB40}.push(static_cast<uint8_t>(rf::gr::FOG_NOT_ALLOWED));
constexpr uint8_t FOG_TYPE = static_cast<uint8_t>(rf::gr::FOG_NOT_ALLOWED);
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this constant really helps with anything and only makes the code longer, because it is obvious from the enum variant name what it is. You use it only in one place, one line later. I can understand if this was a "magic" number, but it is enum variant which logically is a constant too.
At least it should have more specific name. This is not the fog type, this is a fog type used for text. So maybe it should be named TEXT_FOG_TYPE. This way if there will be a need to change another fog type in this scope (e.g. for images or whatever), there won't be a name conflict.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt uncomfortable with the fact that the the overload was dependent on the cast.

std::vector<std::string> get_adapters() override;
std::set<Resolution> get_resolutions(unsigned adapter, unsigned format) override;
std::set<unsigned> get_multisample_types(unsigned adapter, unsigned format, bool windowed) override;
std::set<uint32_t> get_multisample_types(uint32_t adapter, uint32_t format, bool windowed) override;
Copy link
Owner

Choose a reason for hiding this comment

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

all other functions use unsigned adapter so now code is inconsistent and messy. I suggest to cancel those type changes on method parameters

@is-this-c is-this-c requested a review from rafalh December 30, 2025 03:45
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