WIP/PoC: Batch api ergonomics

[vorner]

Hello

As mentioned in #197, I've sketched some API how the batch dispatching could be improved so it's less awkward and doesn't require unsafe.

This is very much a PoC/WiP, I don't want to merge it as it is. I know there are missing docs, copy-pasted comments that are out of context, extra includes that are no longer needed, some things are public when in fact shouldn't be and in general a lot of garbage. I plan to clean it up before asking for it to get merged.

What I would like now is to have some feedback on the API. I have some specific questions, but any other suggestions are fine too:

• Naming. In particular, the simpler case where it is decided up-front how many times the sub-dispatcher should run is currently called multi-batch. I think this is a bad name, because it suggests it's somehow „more“ than the usual but more complex case.
• The multi-batch is a separate method of the builder. Alternatively, it could be exposed as the wrapper type and one could create it directly. I'm not sure on what's better.

In general, however, I feel like this makes the API more approachable (obviously, after it gets some docs and examples).

@azriel91 Do you think I should continue in this? (and, btw, there's a tiny doc update PR from me sitting there for some time as well).

[vorner]

@azriel91 or anyone, I'd really like to get a bit of feedback before continuing to work on it 😇

Btw, if you want to have a look how it feels in actual use in some game (if I can call my attempt so), here it is: https://github.com/vorner/thrust/commit/7b1d0f0de70b1eed98faff99f1ee8d1f14976984. Indeed a lot of boilerplate is gone (as my use case was pausing the game while still continuing with drawing and similar).

[azriel91]

heya, apologies for the radio silence -- am in a really time constrained situation right now. Shall try to get back to this in a week or so

[azriel91]

hey @vorner, are you still looking to use this? Would like to unblock you if possible; I'm not sure the MultiDispatchController as is is useful for general purpose API, but I'm not a user so can't be a good judge.

(sorry, haven't really been a good maintainer)

[vorner]

Well, that game is more fun to write than play and likely won't ever ship (at least not any time soon), so running it against my git fork is fine for now ‒ I'm not blocked by it.

Nevertheless, I feel like I'd welcome change like this in the API (or, actually, addition, as the current way is not prevented), because it allows me to do what I wont in much shorter code and without unsafe. I guess using it for conditional sub-dispatcher (run once or not at all) would be the most common use case of this, but returning usize instead of bool to allow running multiple times also felt „easy“ and adds some flexibility.

But I'm not sure about the exact APIs and naming ‒ that's why it is a WIP. I'll welcome any suggestions as to how to make it nicer.

[vorner]

Hello? Do you see a way forward? Does this crate still have future, now that amethyst migrated off specs? Do you have any suggestions, or should I try some cleanups or anything?

[azriel91]

Heya, shall try and answer as best as possible:

• Do you see a way forward?

  Given I haven't been spending very much time in open source land for a while, the best way is adding you as a maintainer, with @WaDelma's blessing of course. I think that given you're a user, you understand best what is needed; and am sure what changes you do will be of quality.

• Does this crate still have future, now that amethyst migrated off specs?

  I believe this crate and specs are both quite solid, and even if amethyst doesn't use it; specs and shred are good libraries to use on their own. They haven't evolved much since not many people have needed changes (so much as to be actively requesting them); though here we are.

• Do you have any suggestions, or should I try some cleanups or anything?

  Hope this honesty is alright -- I don't think I'm able to spend time on shred (at least for these few months). But to move forward, my general guideline is:

  • The user should be able to intuitively use the library, and not require special / verbose syntax as much as possible.
  • There should be sensible defaults, but a builder-style API to alter those defaults in case the user needs to change them.
  • If possible, the data structures should prevent the user from creating runtime errors

  and again, since you'll be using the API, and we're discussing design + quality, am happy for you to be a maintainer -- am sure you'll be great :bowing_man:

[vorner]

Hmm, I'm not sure about that, really :-(

For one, I haven't used the library since then. I don't do much game development, it was just a side project. I mostly wanted to finish this PR because I've already started it.

The other problem I see is, I already try to maintain more libraries than I have time for. I'm afraid I wouldn't give the library all the care it might need. So if the options are nearly no time (you) and very little time (me) as some kind of attempt to save it/keep it alive, then it could work, but I would say there should be more (like moving the development forward) than just watching if there's a bug reported.

[azriel91]

I mostly wanted to finish this PR because I've already started it.

Gotcha, let's do this one final push for this PR, and then we both get our break.

I had a read through the code and kind of understand it. Have to come back to it later today to see what suggestions I can make; the code structure itself wasn't hard to grasp

[vorner]

I still have to clean up some imports and go over all the docs (not only the ones you've pointed out, but there're bunch of outdated ones in there). I plan to do it this weekend.

(Just letting you know that this is not the "final" version yet, I just had little bit of time)

I guess after the review, I'll have to rebase & rebuild the branch (there are WiP commits and collisions).

[vorner]

I've added the docs, I hope I have overlooked nothing important. The commit messages are bit lacking, but I plan to fold them into the other commits before merging anyway.

Could you give it a look and say if you feel something is still missing or wrong or if we could merge it?

Thanks!

[azriel91]

Heya, give me a few more days -- I didn't manage to get to it this weekend (sorry :bow:).

[vorner]

The current changes:

• Removed the extra comment.
• Added changelog entry (I hope I'm following the conventions correctly).
• Squashed the history (I originally envisioned having two commits with, but I was unable to sort the cleanups into them correctly, so I've at least provided a better commit message for the one resulting comment).
• Rebased on the current master to resolve the conflict.

So there were no „big“ changes and I believe the only thing worth reviewing is the changelog entry.

[azriel91]

Thanks! Shall release this today

[azriel91]

bors r+

[bors[bot]]

Timed out.

[azriel91]

bors retry

[bors[bot]]

Timed out.

[azriel91]

bors retry

[bors[bot]]

Timed out.

[vorner]

Is it timing in the tests (in some way I might be responsible for), or is it timing in submitting the tests to CI? I don't seem to be able to get to any kind of logs :-(.

[azriel91]

ah no, I think it's just there's no .travis.yml for this repo. (should've checked instead of blindly re-running).

Wait a minute, there is (I'm sure I looked for it yesterday and couldn't find). Shall look into it properly again.

[azriel91]

Hm, the build passes on travis. Maybe bors isn't picking that up.

Shall do one now to see if "now" makes a difference to yesterday.

bors retry

[azriel91]

Looks like it's travis that is taking a long time to begin a build. I'll see whether I can switch CI to github actions and get bors happy with that.

Trying in https://github.com/amethyst/shred/pull/201.

[azriel91]

bors cancel

[bors[bot]]

Timed out.

[azriel91]

@WaDelma I'm alright with bypassing bors to get this through since it passes on travis

I couldn't figure out why bors is timing out. #201 switches CI to github actions, which also has bors' timeout (despite the CI check finishing quickly).

do you have any other suggestions?

[WaDelma]

@azriel91 I unfortunately don't have much knowledge on bors. If I click "view details" on the timed out bors job I get permission denied, so cannot really debug it. Dunno what to do except skip bors.

[azriel91]

Let's go.

[vorner]

Thank you :-)