Skip to content

Add Error State to Storages#776

Merged
kvark merged 3 commits intogfx-rs:masterfrom
kunalmohan:error-state
Jul 10, 2020
Merged

Add Error State to Storages#776
kvark merged 3 commits intogfx-rs:masterfrom
kunalmohan:error-state

Conversation

@kunalmohan
Copy link
Copy Markdown
Contributor

@kunalmohan kunalmohan commented Jul 10, 2020

Connections
Link to the issues addressed by this PR, or dependent PRs in other repositories
fix #772

Description
Describe what problem this is solving, and how it's solved.
This aims to replace VecMap with a simple Vec<Element<T>> to keep track of valid and error Ids in Storages. This lays the foundations for #638. A new enum Element<T> is introduced as described in the linked issue.
The Error Variant is not used anywhere at the moment.

Testing
Explain how this change is tested.
A Successful build is the only check done at the moment.

Copy link
Copy Markdown
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Copy Markdown
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Copy Markdown
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Copy Markdown
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

Thank you for taking a stab at this!

I am becoming worried here. If suddenly any access to anything is going to be able to return an error.
Then there is this contagious aspect: say a texture became invalid at some point, and then we access the view of it at some later point. Technically, we should turn the view into error state too, but how and when are we going to even know that there is a view that needs to be errored?

I need to think about this more and talk to the group maybe.

@kvark kvark mentioned this pull request Jul 10, 2020
Copy link
Copy Markdown
Contributor

@monocodus monocodus bot left a comment

Choose a reason for hiding this comment

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

The .monocodus config not found in your repo. Default config is used.
Check config documentation here

Copy link
Copy Markdown
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

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

I believe my concerns are all addressed now.
Thank you!

@kvark kvark merged commit 26dcdaa into gfx-rs:master Jul 10, 2020
@kunalmohan
Copy link
Copy Markdown
Contributor Author

I didn't squashed the commits (was waiting for the final approval). But I guess that was okay..so cool.

@kunalmohan kunalmohan deleted the error-state branch July 10, 2020 18:34
This was referenced Jul 11, 2020
bors bot added a commit that referenced this pull request Jul 11, 2020
779: Fix hub insertion r=kvark a=kvark

**Connections**
Fixes #778 
Follow-up to #776

**Description**
Problem was `resize_with` takes new length, not a difference.

**Testing**
Untested

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
@kvark
Copy link
Copy Markdown
Member

kvark commented Jul 11, 2020

@kunalmohan now with fast CI this becomes less of a problem to just merge stuff outside of bors. So I squashed them on merge.

bors bot added a commit that referenced this pull request Jul 11, 2020
782: Add check while trying to remove uninserted Ids r=kvark a=kunalmohan

**Connections**
_Link to the issues addressed by this PR, or dependent PRs in other repositories_
An attempt to fix #781 
regression from #776 

**Description**
_Describe what problem this is solving, and how it's solved._
When we used `VecMap`, it simply returned `None` for even out of bounds access to the map (We depended on it returning `None`). After #776 , we get a panic. So adding a simple index check before accessing it fixes this issue.

**Testing**
_Explain how this change is tested._
Tested on wgpu-rs examples with the changes in gfx-rs/wgpu-rs#430. All examples run fine except the `cube` which segfaults.
<!--
Non-trivial functional changes would need to be tested through:
  - [wgpu-rs](https://github.com/gfx-rs/wgpu-rs) - test the examples.
  - [wgpu-native](https://github.com/gfx-rs/wgpu-native/) - check the generated C header for sanity.

Ideally, a PR needs to link to the draft PRs in these projects with relevant modifications.
See #666 for an example.
If you can add a unit/integration test here in `wgpu`, that would be best.
-->


Co-authored-by: Kunal Mohan <kunalmohan99@gmail.com>
bors bot added a commit that referenced this pull request Jul 12, 2020
783: Basic use of Error for bind groups r=kvark a=kvark

**Connections**
Follow up to #776 

**Description**
Trying to tidy up a few things and add a public use of `Error` variant

**Testing**
not tested

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
bors bot added a commit that referenced this pull request Aug 4, 2020
857: Separate valid internal IDs from external ones r=cwfitzgerald a=kvark

**Connections**
Closes #638 
wgpu-rs update - gfx-rs/wgpu-rs#494

**Description**
The core change here is to allow user-specified IDs to be in the "Error" state that was introduced in #776 .

This is done by defining an internal `Valid<I>` wrapper. Now, the hub storages can be indexed by this valid wrapper only. For regular IDs, we have to go through `storage.get(index)`, which returns a `Result`. It still panics if the ID is totally garbage though, we don't want to handle what can't be expected here.

All the other changes come mostly as a consequence of that:
  - new "Invalid*" error variants are added
  - the error types are undergone sever refactoring
  - new `command/draw.rs` module for stuff shared between render passes and bundles
  - functions to generate error IDs for all the types
  - various renames, e.g. `comb` -> `cmd_buf`

The expected use by wgpu-rs is unchanged. So far, I don't think we'd be generating Error IDs, but we can always reconsider.
For browsers though, if `device_create_xxx` failed, they would generate and error ID. It will occupy the slot up until the corresponding JS object is dropped.

**Testing**
Tested on wgpu-rs examples

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
kvark added a commit to kvark/wgpu that referenced this pull request Jun 3, 2021
776: Update wgpu with the new vertex format r=kvark a=kvark

Based on gfx-rs#1235

Co-authored-by: Dzmitry Malyshau <kvarkus@gmail.com>
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.

Support Error state in storages

2 participants