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

Replace object visual by enum #15681

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Jan 15, 2025

This replaces the visual variable of the ObjectProperties class by an enum.

  • It gets rid of some string comparison and can even use a switch statement, which is good, I guess, but I mainly want this for Only send changed object properties #15671.
  • It still gets serialized as a string.
  • A wanted to use an enum class, but replaced it with an enum, because it would need a lot of explicit int casts in order to use our EnumString. Also, I did not put it in a namespace instead of the OBJECTVISUAL_ prefix, since this way it is more similar to the existing code, e.g. the HUD enums. Tell me in case you prefer something else.

To do

Ready for Review.

How to test

👁️ 👁️

@cx384 cx384 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Jan 15, 2025
@cx384 cx384 marked this pull request as ready for review January 15, 2025 18:55
@cx384 cx384 force-pushed the object_visual_enum branch from d5757fe to 0e30370 Compare January 15, 2025 19:34
src/object_properties.cpp Outdated Show resolved Hide resolved
src/script/common/c_content.cpp Outdated Show resolved Hide resolved
@cx384
Copy link
Contributor Author

cx384 commented Jan 16, 2025

Thanks for the review, it should be fixed now.

@cx384 cx384 mentioned this pull request Jan 16, 2025
3 tasks
src/object_properties.h Outdated Show resolved Hide resolved
src/client/content_cao.cpp Outdated Show resolved Hide resolved
src/object_properties.cpp Outdated Show resolved Hide resolved
@cx384 cx384 force-pushed the object_visual_enum branch from b0b9c7c to 543713e Compare January 19, 2025 13:16
@sfan5 sfan5 self-requested a review January 19, 2025 18:09
@cx384 cx384 force-pushed the object_visual_enum branch from e7565ec to 4525e12 Compare January 25, 2025 13:58
@cx384
Copy link
Contributor Author

cx384 commented Jan 25, 2025

Rebased

@nerzhul
Copy link
Contributor

nerzhul commented Jan 27, 2025

unfortunately another rebase is required. I'm totally in favor of such quality change.

@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author label Jan 27, 2025
@cx384 cx384 force-pushed the object_visual_enum branch from 4525e12 to 4af592e Compare January 27, 2025 11:27
@cx384 cx384 removed the Rebase needed The PR needs to be rebased by its author label Jan 27, 2025
src/client/content_cao.cpp Outdated Show resolved Hide resolved
src/object_properties.cpp Show resolved Hide resolved
src/object_properties.cpp Outdated Show resolved Hide resolved
src/script/common/c_content.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 self-requested a review January 29, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants