Implement support for RID by lilizoey · Pull Request #171 · godot-rust/gdext (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation24 Commits1 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

RID is here implemented as an enum

pub enum Rid { Valid(NonZeroU64), Invalid, }

This is to take advantage of null-pointer optimization, while bringing type safety in.
Option would not be usable for this without bigger changes in the code, as we cannot easily implement traits on Option<Rid>, and it'd take a lot of work in the codegen to change the right function parameters and return types to Option<Rid>/Rid where appropriate.

Closes #102

Choose a reason for hiding this comment

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

Thank you for the PR!

As suggested in comments, I would start with a smaller, less redundant API surface.

Also, some of the tests you add do a lot of bruteforcing over a large design space. Do they increase overall test duration (in debug mode), and if so, how by much?

Comment on lines 20 to 22

/// # Layout:
///
/// `Rid` has the same layout as a [`u64`], where [`Rid::Invalid`] is 0, and [`Rid::Valid(i)`] is `i`.

Choose a reason for hiding this comment

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

I don't think we should expose this implementation detail, it only limits us in the future (e.g. if we ever add a debug-only field for extra validation or so). We already have new and to_u64 functions that are reasonably cheap.

Choose a reason for hiding this comment

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

Also, as far as I know, without #[repr(C)] and #[repr(transparent)], no layout is guaranteed, even if with the enum optimization you mentioned. It's very unlikely to change, but a compiler is theoretically free to do so.

Choose a reason for hiding this comment

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

Also, as far as I know, without #[repr(C)] and #[repr(transparent)], no layout is guaranteed, even if with the enum optimization you mentioned. It's very unlikely to change, but a compiler is theoretically free to do so.

The rustnomicon explicitly says the nullable pointer optimization is guaranteed:

This is called an "optimization", but unlike other optimizations it is guaranteed to apply to eligible types.

And considering the fields of the enum, there is only one possible layout.

Choose a reason for hiding this comment

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

I also just ran the itests with -Zrandomize-layout and it works. If the layout wasn't guaranteed here, then rid_equiv should fail under that. Since it checks that Rid::Invalid maps to 0, and Rid::Valid(i) maps to i (for one specific i at least)

Choose a reason for hiding this comment

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

-Zrandomize-layout passing is a necessary but not sufficient criterion for correct layout -- it only reorders fields and doesn't change fields etc. But to my understanding, it's conforming for a Rust compiler to e.g. add padding bytes at the end, even if the enum optimization is available. But we're really talking about theory here, I also don't see why this would happen.

Anyway, it doesn't matter much: the point is that a guaranteed layout limits potential implementation changes in the future, so we shouldn't document it. There's the existing API for such conversions.

/// Returns `true` if this is an invalid RID.
#[inline]
pub const fn is_invalid(&self) -> bool {
matches!(self, Rid::Invalid)
}

Choose a reason for hiding this comment

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

Providing negated versions of accessors is not something we typically do. I don't think we should start a precedent here, as this will lead to discussions in which cases it's OK or not. Given that RIDs validity checks should ideally not be very common, it's fine if a user has to explicitly type a !.

Choose a reason for hiding this comment

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

oh fair, i think the c++ code had it so i just added it. But it does follow the precedent of Option which has both is_some and is_none.

Choose a reason for hiding this comment

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

But it does follow the precedent of Option which has both is_some and is_none.

Yes, and it does not follow the precedent of Vec::is_empty() which has no such opposite. Or any other type in godot-rust, which is much more relevant here 🙂

Choose a reason for hiding this comment

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

Vec isn't an enum though, i chose to compare it with Option because they're both similar enums, and having an is_x function for each enum variant is not uncommon.

Comment on lines 88 to 82

impl From for Rid {
#[inline]
fn from(id: u64) -> Self {
Self::new(id)
}
}

impl From for Rid {
#[inline]
fn from(id: NonZeroU64) -> Self {
Self::Valid(id)
}
}

impl From for u64 {
#[inline]
fn from(rid: Rid) -> Self {
rid.to_u64()
}
}

impl TryFrom for NonZeroU64 {
type Error = ();

#[inline]
fn try_from(rid: Rid) -> Result<Self, Self::Error> {
rid.to_non_zero_u64().ok_or(())
}
}

impl From for Option {
#[inline]
fn from(rid: Rid) -> Self {
match rid {
Rid::Invalid => None,
Rid::Valid(id) => Some(id),
}
}
}

impl From<Option> for Rid {
#[inline]
fn from(id: Option) -> Self {
match id {
Some(id) => Rid::Valid(id),
None => Rid::Invalid,
}
}
}

Choose a reason for hiding this comment

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

Again, those are redundant with the inherent methods, and I'd prefer to have explicit versions rather than 323.into(). It's also not very common that a user is actually interested in the integer itself. Rid is mostly intended to be an opaque ID, and there are already named conversions for the other cases.

Comment on lines 16 to 22

/// Disable printing errors from Godot. Ideally we should catch and handle errors, ensuring they happen.
/// But that isn't possible, so for now we just disable printing the error to avoid spamming the terminal.
fn should_error(mut f: impl FnMut()) {
Engine::singleton().set_print_error_messages(false);
f();
Engine::singleton().set_print_error_messages(true);
}

Choose a reason for hiding this comment

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

Given the semantics, this could be named suppress_godot_print rather than should_error.
Would it make sense to move it next to expect_panic?

Comment on lines 55 to 59

let mut v = vec![];
for _ in 0..1000 {
v.push(server.canvas_item_create());
}
v

Choose a reason for hiding this comment

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

Comment on lines +84 to +74

#[itest]
fn strange_rids() {

Choose a reason for hiding this comment

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

Could you maybe add a quick comment what exactly is being tested here?

Also, some of the tests you add do a lot of bruteforcing over a large design space. Do they increase overall test duration (in debug mode), and if so, how by much?

I haven't noticed much, checking with Instant, multi_thread_test took 0.05262027 seconds and strange_rids took 0.013599553 seconds.

In gdnative, we use Rid::is_occupied instead of is_valid as a name. I find this more expressive, as it doesn't suggest that the RID points to a valid (existing, currently loaded) resource.

We also explicitly documented that difference in meaning, see Rid::is_occupied docs.

Here the situation is a bit different as we also have the enum variants 🤔
Do we gain much by having it as an enum, or should we just provide an easy conversion to Option<u64>? The conversion to Option<NonZeroU64> is technically more correct, but it's inconvenient since that type isn't used anywhere else and forces more verbose conversions on the user side...

In gdnative, we use Rid::is_occupied instead of is_valid as a name. I find this more expressive, as it doesn't suggest that the RID points to a valid (existing, currently loaded) resource.

Honestly to me is_occupied even more suggests that the RID points to a resource than is_valid. I dont mind the name but to me it reads as "this RID is occupied by a resource".

Here the situation is a bit different as we also have the enum variants thinking Do we gain much by having it as an enum[?] (...)

As i see it
Advantages:

easy construction of valid/invalid RID
ability to pattern match
more clear to the user what Valid/Invalid means

Disadvantages:

must make fields public
relies on technically not guaranteed (but highly unlikely to break in our use-cases) type-layout
must use NonZeroU64

(...) or should we just provide an easy conversion to Option<u64>? The conversion to Option<NonZeroU64> is technically more correct, but it's inconvenient since that type isn't used anywhere else and forces more verbose conversions on the user side...

The functions to convert the RID to/from integers are likely not gonna be used much except in the cases where it is useful to maintain the distinction between u64 and NonZeroU64, such as implementing your own custom servers. So of those two i'd advocate for Option<NonZeroU64> at least.

I'm not generally opposed to enums, but it feels a bit strange that InstanceId and Rid now have completely different APIs, although they are conceptually very similar. This is also why I was advocating the Option first, but I see how that's hard in this case, as Rid is used in all the generated APIs.

But maybe this can be unified later; it's good to already have a first implementation.

That said, with the currently proposed API it's quite verbose to get a Option<u64> out of an Rid (which gives you both the underlying type and the knowledge whether it's occupied).

rid.to_non_zero_u64().map(NonZeroU64::get);

if rid.is_valid { Some(rid.to_u64()) } else { None }

match rid { Valid(nz) => Some(nz.get()) Invalid => None }

NonZeroU64 is nice in theory, just like NonNull, when you want to have very expressive, type-safe APIs.
However, in practice they're often annoying to work with, à la "get the raw type with extra steps".

So there are three solutions to that i can think of:

add a function to_option_u64 (or some other name)
make to_u64 return Option<u64> (and rename original?)
add a From<Rid> for Option<u64> impl (and the reverse?)

Which do you prefer?

Maybe 1. as try_to_u64?

I don't think we need to_non_zero_u64 then, if we already have NonZeroU64 in the enumerator. Most people are not likely to use this type.

Maybe to_u64 should in that case be called something like to_zero_u64 (I don't know a better name right now) 🤔

What about this? to_u64 and to_valid_u64

Add RenderingServer to minimal classes, to be able to test the RID impl against an actual server Add Engine to minimal classes, to be able to disable error printing in itests

Thanks!

I'll merge this, however expect significant API changes as I'll try to make InstanceId and Rid more consistent with each other. I will also have another look at the naming at that time, so I think it's good to have something for now. I still believe "valid" should be "occupied", for example.

bors r+

2 participants