WIP/PoC: Batch api ergonomics #198
Conversation
|
@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: vorner/thrust@7b1d0f0. Indeed a lot of boilerplate is gone (as my use case was pausing the game while still continuing with drawing and similar). |
|
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 |
|
hey @vorner, are you still looking to use this? (sorry, haven't really been a good maintainer) |
|
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. |
|
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? |
|
Heya, shall try and answer as best as possible:
|
|
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. |
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 |
|
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). |
|
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! |
|
Heya, give me a few more days -- I didn't manage to get to it this weekend (sorry 🙇). |
* The dispatcher is now passed in as a value, so it can contain data. * No need to touch unsafe by the user. * Helper for the usual case where the number of times the batch is run is decided ahead of the time. Closes #197
|
The current changes:
So there were no „big“ changes and I believe the only thing worth reviewing is the changelog entry. |
|
Thanks! Shall release this today |
|
bors r+ |
198: WIP/PoC: Batch api ergonomics r=azriel91 a=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). Co-authored-by: Michal 'vorner' Vaner <vorner@vorner.cz>
|
Timed out. |
|
bors retry |
198: WIP/PoC: Batch api ergonomics r=azriel91 a=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). Co-authored-by: Michal 'vorner' Vaner <vorner@vorner.cz>
|
Timed out. |
|
bors retry |
198: WIP/PoC: Batch api ergonomics r=azriel91 a=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). Co-authored-by: Michal 'vorner' Vaner <vorner@vorner.cz>
|
Timed out. |
|
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 :-(. |
|
Wait a minute, there is (I'm sure I looked for it yesterday and couldn't find). Shall look into it properly again. |
|
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 |
198: WIP/PoC: Batch api ergonomics r=azriel91 a=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). Co-authored-by: Michal 'vorner' Vaner <vorner@vorner.cz>
|
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 #201. |
|
bors cancel |
|
Timed out. |
|
@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. |
|
Let's go. |
|
Thank you :-) |
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:
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).