Remove the Destroyed state from Storage

[nical]

Connections

Depends on #4896

Description

Removes the logic to express destroyed resources in the registry, since we have moved to handling it in the resources themselves.

Checklist

• Run cargo fmt.
• Run cargo clippy.
• Run cargo xtask test to run tests.

[teoxoy]

We were previously marking the resources as destroyed immediately after the destroy fn was called and subsequent attempts at getting the resources would fail.

We are now manually checking if the hal resource was destroyed which won't be the case until later. It also looks like we don't check this after every {textures,buffers}.get(id) anymore.

It seemed that Element::Destroyed and Element::Error were both equivalent with the invalid flag in the spec and we haven't yet fully moved invalidity inside objects.

[nical]

There are a lot of places where the check is now happening by getting an Option out of resource.raw(&snatch_guard) which is None if the resource is destroyed (basically anywhere we actually use the thing). So we only need to manually check when usage of the resource is deferred to some time after the validation is supposed to happen.

I personally preferred the way the registry was providing this check transparently, but since our plan is to get rid of the registry altogether, doing the checks manually is the the way forward.
If you see places where the check is missing, let me know.

Moving the Element::Error state to the object is going to be a rather tedious task but we'll have to do that too at some point.

[teoxoy]

I see. I think that removing Element::Destroyed while we still don't have proper object invalidation makes things harder to reason about for those cases that we have to manually check if the resource hasn't been destroyed earlier than accessing said resource.

I'm not very well-versed in this area of the code, I'll let someone else also chime in.

[nical]

What do you envision for proper object validation? Is it some missing if resource.is_destroyed(&snatch_guard) { branches or are you thinking of some system for checking it in a more automatic way?

[teoxoy]

I think we'll need a system to do it in a more generic way since almost all API calls check for validity (and almost all objects can be invalid).

https://gpuweb.github.io/gpuweb/#abstract-opdef-valid-to-use-with

Element::Destroyed & Element::Error seemed to be equivalent to object validity but we now (with the plan of moving the storages out) need to move this state in the objects themselves (as Arc<Option<T>>?).

[nical]

Element::Destroyed & Element::Error seemed to be equivalent to object validity but we now (with the plan of moving the storages out) need to move this state in the objects themselves (as Arc<Option>?).

Yep. Probably something like that, or something along the lines of Option<Arc<T>> to encourage checking early and keeping the arc which we don't need to check again in later stages. The initial error state is easier because it is set once at creation.
Texture and buffer destroyed state is more complicated because we have to check it everywhere it is used in addition to where standard validation happens, because the resource may be marked destroyed at any time.

[nical]

Ci is stuck because github had a cold the other day but this is ready to land.

[teoxoy]

It probably needs a rebase since it's waiting for "Test Mac aarch64" which was recently added.

[cwfitzgerald]

Minor comment - take or leave, g2g after.