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

Remove the check for WrongNumberOfInstances #5399

Merged
merged 3 commits into from
Mar 5, 2024

Conversation

jleibs
Copy link
Member

@jleibs jleibs commented Mar 5, 2024

What

Also removed instances of the restriction in documentation.

I believe, in theory this means we can remove a bunch of places where we determine and pass around num_instances. However, doing that requires cleaning up the splat-determination logic and the special splat-instance-key-value, which I assume will come as part of:

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG
  • If applicable, add a new check to the release checklist!

@jleibs jleibs added 🪵 Log & send APIs Affects the user-facing API for all languages include in changelog labels Mar 5, 2024
@jleibs jleibs marked this pull request as ready for review March 5, 2024 13:51
@Wumpf Wumpf self-requested a review March 5, 2024 13:59
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

was expecting some left overs on https://www.rerun.io/docs/concepts/entity-component but couldn't find any mention of that now lifted restriction there.
Off it goes! 🚢

@teh-cmc
Copy link
Member

teh-cmc commented Mar 5, 2024

I believe, in theory this means we can remove a bunch of places where we determine and pass around num_instances. > However, doing that requires cleaning up the splat-determination logic and the special splat-instance-key-value, which I assume will come as part of #5303

Yep; at a first glance it's even likely that num_instances disappears from the wire entirely

@jleibs jleibs merged commit 9f29bd0 into main Mar 5, 2024
40 of 47 checks passed
@jleibs jleibs deleted the jleibs/remove_wrong_number_check branch March 5, 2024 14:18
jleibs added a commit that referenced this pull request Mar 5, 2024
### What
Cherry-picked over the Name / Visibillity changes from
`andreas/serializable-spaceviewblueprint`, so we'll need to navigate
merge conflicts there.

Standard implementation of serializers and a unit-test for them:

After talking to @Wumpf I moved some of the list-types up a level to
simplify the serialization code, but this raised an issue with
mismatched dimensions. Need to discuss if we want to remove this check
(in light of the instance-key simplification), or push things back down
to mono-components with internal lists.

### TODO

- [x] Dragging onto a Grid container breaks due to the mismatched
dimensions between the contents and the row-shares.
  - Fixed by: #5399
```
[2024-03-04T23:10:53Z WARN  re_log::result_extensions] crates/re_viewport/src/container.rs:240 Failed to create Container blueprint.: Each cell must contain either 0, 1 or `num_instances` instances, but cell 'rerun.blueprint.components.RowShare' in '/container/439ad7ad-829c-4085-833b-eb30efae0810' holds 2 instances (expected 3)
```

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5390/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5390/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5390/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG
* [x] If applicable, add a new check to the [release
checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)!

- [PR Build Summary](https://build.rerun.io/pr/5390)
- [Docs
preview](https://rerun.io/preview/6964d9643faba86a734746885f4a8792da3685f1/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/6964d9643faba86a734746885f4a8792da3685f1/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Andreas Reich <r_andreas2@web.de>
@emilk emilk changed the title Remove the check for WrongNumberOfInstances Remove the check for WrongNumberOfInstances Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog 🪵 Log & send APIs Affects the user-facing API for all languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants