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

Nicer enum and enumerator names #562

Merged
merged 7 commits into from
Jan 13, 2024
Merged

Nicer enum and enumerator names #562

merged 7 commits into from
Jan 13, 2024

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Jan 10, 2024

Important

The API may still change, as there are more improvements to enums planned. In particular, we are considering moving to the PascalCase naming convention, but there are a few drawbacks as well.

If you want to avoid possibly multiple breaking changes, use the now-deprecated names for some time.

Changes

This PR affects generated code only.

Enums in the godot::engine module now follow the PascalCase naming convention, which classes already use:

  • SDFGIYScale -> SdfgiYScale
  • ASTCFormat -> AstcFormat
  • EnvironmentSSAOQuality -> EnvironmentSsaoQuality

Enumerator names are shortened, by stripping redundant information that's already contained in the enum.
This reduces some excessively long expressions:

  • ColorDefault.COLOR_DEFAULT_WHITE ->
    ColorDefault.WHITE

  • ViewportDebugDraw.VIEWPORT_DEBUG_DRAW_INTERNAL_BUFFER ->
    ViewportDebugDraw.INTERNAL_BUFFER

  • DefaultCanvasItemTextureRepeat.DEFAULT_CANVAS_ITEM_TEXTURE_REPEAT_DISABLED ->
    DefaultCanvasItemTextureRepeat.DISABLED

  • ParticlesCollisionHeightfieldResolution.PARTICLES_COLLISION_HEIGHTFIELD_RESOLUTION_2048 ->
    ParticlesCollisionHeightfieldResolution.RESOLUTION_2048
    (includes second-to-last part because "2048" would be invalid)

But even exotic examples aside, it makes even simple use cases much more readable.
Commit 232b5d7 demonstrates just how much gdext's own code benefits from this change.

Being a heuristic, this is obviously not perfect, but I'd say the algorithm is quite decent. It also takes into account common abbreviations like Param -> Parameter, Op -> Operator, etc.

Migration

Old enums and enumerators that are affected by this change continue to work for now; they are marked deprecated and will eventually be removed. This should allow a smooth migration over time.

For enumerators, the old names are still searchable in docs via #[doc(alias)]; this should help discoverability.

@Bromeon Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Jan 10, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-562

@Bromeon Bromeon force-pushed the feature/nice-enums branch from a7009c2 to 232b5d7 Compare January 10, 2024 23:27
@Bromeon
Copy link
Member Author

Bromeon commented Jan 11, 2024

I tried an alternative algorithm, which seems much simpler: from a set of enumerators, establish a common prefix that is shared among all them.

That would not require looking at the enum name at all and could also detect patterns like PARAM_ without being explicitly told so. Now the downside: Godot's inconsistency also reaches here.

3 different prefixes (this would mean that only ARRAY_ is common):

ArrayFormat.FORMAT_CUSTOM1_SHIFT
ArrayFormat.FORMAT_CUSTOM2_SHIFT
ArrayFormat.FORMAT_CUSTOM_MASK
ArrayFormat.COMPRESS_FLAGS_BASE
ArrayFormat.FLAG_USE_2D_VERTICES
ArrayFormat.FLAG_COMPRESS_ATTRIBUTES

Omission of possibly (?) important information:

DriverResource.VULKAN_QUEUE        -> DriverResource.QUEUE
HostStatistic.TOTAL_RECEIVED_DATA  -> HostStatistic.RECEIVED_DATA   

It breaks down if there is only a single enumerator:

PathfindingAlgorithm.PATHFINDING_ALGORITHM_ASTAR

If numbers appear at the beginning, we'd need to differentiate per enumerator (like now), or force the prefix on all again:

KEY_SEMICOLON -> SEMICOLON
KEY_9         -> KEY_9

Enumerators with inconsistent prefixes are not recognized:

METHOD_FLAG_STATIC 
METHOD_FLAGS_DEFAULT

But ultimately it seems a bit more robust. Of course this could become endlessly complex (like combining both algorithms), but I don't think it's worth to strive for perfection here😉

@0awful
Copy link
Contributor

0awful commented Jan 11, 2024

I'm in favor of us embracing the rust convention of pascal case. This looks like a good direction. 👍

@Bromeon Bromeon force-pushed the feature/nice-enums branch from 232b5d7 to de7d2de Compare January 13, 2024 16:38
…duced from enum

Slightly simpler implementation, less explicit special cases and overall better results.
Available only with features `codegen-full` and `experimental-godot-api`.
Remove preceding mapping #[test] in godot-codegen.

Also re-enable accidentally excluded codegen_test.rs.
@Bromeon Bromeon force-pushed the feature/nice-enums branch from de7d2de to 51bea46 Compare January 13, 2024 17:00
@Bromeon Bromeon enabled auto-merge January 13, 2024 17:08
@Bromeon Bromeon added this pull request to the merge queue Jan 13, 2024
Merged via the queue into master with commit 52628a0 Jan 13, 2024
@Bromeon Bromeon deleted the feature/nice-enums branch January 13, 2024 17:19
@Bromeon
Copy link
Member Author

Bromeon commented Jan 13, 2024

On PascalCase for enumerators

Once we turn Godot enums into actual Rust enums, Rust convention would be to use PascalCase instead of SCREAM_CASE for the enumerators. This is relatively easy to achieve in codegen, however there are some caveats:

  1. Technically, bitfields will need to stay regular const constants, not enums. Having them SCREAM_CASE but Godot enums PascalCase would be a weird inconsistency.

    • Of course, we could just make them PascalCase as well.
  2. There are some extreme examples that are entirely unreadable in PascalCase, and actively remove valuable information (separators). These are rather rare though.

    // RenderingDevice class
    DataFormat::G12X4_B12X4_R12X4_3PLANE_420_UNORM_3PACK16
    DataFormat::G12x4B12x4R12x43plane420Unorm3pack16
  3. While PeerState::DisconnectLater is slightly nicer to look at than PeerState::DISCONNECT_LATER, the latter is closer to GDScript's STATE_DISCONNECT_LATER.

    • Since we already remove prefixes, this may make it a bit easier for users to recognize known identifiers.
  4. These enums are recognizable as generated Godot constants/bitfields/enums, rather than manual ones.

    • This point was already mentioned for get_ prefixes; in other words, it's OK to deviate from Rust if it helps staying closer to Godot.
    • There are some semantical specialties to take into account, e.g. that new enumerators may be added in the future.

Final decision is still open. As mentioned on top of this PR, if you don't want to risk multiple API changes, either wait with updating or use the old, now-deprecated symbols for the time being.

There is currently a branch qol/enumerator-pascal-case which explores that. See diff from master.

@fpdotmonkey
Copy link
Contributor

With respect to (2), I think it would be fine to treat those as exceptional. Have PascalCase most places and SCREAM_CASE just for those crazy enums. Though it should be that all variants of the same enum follow the same convention, so for the RenderingDevice::DataFormat example, ::DATA_FORMAT_MAX should be SCREAM_CASE event though it would otherwise be fine in PascalCase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: engine Godot classes (nodes, resources, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants