Existing PR's

A quieter space for design discussion of long-term projects
Locked
FluffyFreak
Posts: 1343
Joined: Tue Jul 02, 2013 1:49 pm
Location: Beeston, Nottinghamshire, GB
Contact:

Existing PR's

Post by FluffyFreak »

Because it's frustrating...

Just need reviewing:
https://github.com/pioneerspacesim/pioneer/pull/2491 - Shields - originally done after this topic: http://pioneerspacesim.net/forum/viewtopic.php?f=3&t=68.
https://github.com/pioneerspacesim/pioneer/pull/2529 - Hud Trails - like the speed lines feature but draw a trail that follows other ships, needs review and some testing - no-one has commented yet.
https://github.com/pioneerspacesim/pioneer/pull/2570 - Space texture backdrop - backported from Paragon, brought into line with Pioneer way of doing stuff, has decent sized cubemap and I want to extend it in future but only after/if this gets merged.

Controversial:
https://github.com/pioneerspacesim/pioneer/pull/2604 - Aggresively populate the sector cache using threads, also make pruning the StarSystemCache less aggressive by making it depend on current location. The idea being that before we need a Sector or StarSystem it should already be cached for us. My testing shows that it works ok but getting a feel for the performance improvement is much harder since it rely's on not being able to measure a stall that some earlier caching work has already made harder to get.
https://github.com/pioneerspacesim/pioneer/pull/2527 - Luomu's Msg logging changes. I like this one because it fixes something that has been complained about FOR YEARS.
FluffyFreak
Posts: 1343
Joined: Tue Jul 02, 2013 1:49 pm
Location: Beeston, Nottinghamshire, GB
Contact:

Re: Existing PR's

Post by FluffyFreak »

Less ranting.
robn
Posts: 302
Joined: Mon Jul 01, 2013 1:11 am
Location: Melbourne, Australia

Re: Existing PR's

Post by robn »

FluffyFreak wrote:https://github.com/pioneerspacesim/pioneer/pull/2491 - Shields - originally done after this topic: http://pioneerspacesim.net/forum/viewtopic.php?f=3&t=68 - more than 3 months old without feedback.
I started talking to nozmajner about the desirability of this, but I don't recall if we reached a conclusion. I'm guessing not because I didn't write it down.
https://github.com/pioneerspacesim/pioneer/pull/2529 - Hud Trails - like the speed lines feature but draw a trail that follows other ships, needs review and some testing - almost 3 months old without any comment whatsoever.
The mention of a frame glitch put this straight onto my "to check later" list. Which isn't really a list, mostly just a sigh that there's too much to do.

https://github.com/pioneerspacesim/pioneer/pull/2570 - Space texture backdrop - backported from Paragon, brought into line with Pioneer way of doing stuff, has decent sized cubemap and I want to extend it in future but only after/if this gets merged.

Yeah, I promised to do something on this and didn't.
https://github.com/pioneerspacesim/pioneer/pull/2604 - Aggresively populate the sector cache using threads, also make pruning the StarSystemCache less aggressive by making it depend on current location. The idea being that before we need a Sector or StarSystem it should already be cached for us. My testing shows that it works ok but getting a feel for the performance improvement is much harder since it rely's on not being able to measure a stall that some earlier caching work has already made harder to get.
Haven't looked yet, but based on the earlier work I already understood that this was coming, and I'm not expecting there to be any problem.
https://github.com/pioneerspacesim/pioneer/pull/2527 - Luomu's Msg logging changes. I like this one because it fixes something that has been complained about FOR YEARS. If nothing else it makes the game better even if it doesn't do it in the best possible way. I'd rather refactor the code based on this, and benefit from the improvements it brings for the intervening months it takes to get the "perfect" implementation, than stumble along with the current crap pissing me off every single day. It takes code out of the CPan system and makes it a separate system, we can change the display of the messages to use the new GUI later, but in my eyes this has no negatives: it's an improvement on the old messaging system, it's taken out of CPan making itself and CPan easier to rewrite/extend. It's not all good: it's not using the new GUI.
On balance I do think it should be merged because the old message system pisses me off every time I launch the game, every-single-time.
I don't disagree with the concept, or the need for it. It was mostly that it was on the old UI that was the problem.


So in response to the rant, it basically comes down to us having processes that require a handlful of people to work effectively, but pretty much only me to work it. So now I pretty much get to choose between project management or writing code, and I end up doing both fairly badly.

I think about what to do about this nearly every day, but I don't know what to do that doesn't result in either the project dying or me leaving.
  • I can step down from project stuff, ignore the PR queue and just work on code. There's nobody that really wants to pick up all the stuff I do.
  • I can stop writing code and just do project stuff. I've tried that before on a previous project. I burned out and it took a year before I wanted to touch a computer outside of work. I'm rather keen to avoid that.
  • I can lower my standards, mostly just merging anything that comes along. I worry that the lack of direction will kill is (and we do have some direction, believe it or not). I also worry that it'll add more mess to the codebase. That's going to feel too much like hard work being undone, and I won't be able to watch it for long.
  • I can open git access to everyone (and here's some reading on a model that might work). Same danger.
I'm not trying to guilt anyone here. This is supposed to be fun, so I don't want anyone to take on anything in an attempt to make my life easier. I'm just writing this because I genuinely don't know what to do at this point. Something clearly has to change, but every path forward seems to carry a lot of risk of me losing something.

It might not happen; I could open it up and it ushers in a golden age. But the risk is real to me. I've poured three years of my soul into this project. I know that sounds dramatic; I'm just trying to describe how attached I am to this. When I think about letting it go it feels like I'm going to lose an arm. And really, that's the problem.

Someone find me a way out. I don't see how any of the above options can work. Is it time for me to go? (and please don't say "no, please, don't leave us, it'll be ok", because people do say that sort of thing and its not really helpful).

Also btw, I'm not chucking a massive sad. I'm actually in rather a good mood here right now ;)
nozmajner
Posts: 1079
Joined: Mon Jul 01, 2013 3:25 pm
Location: Budapest HU

Re: Existing PR's

Post by nozmajner »

I can't recall the shield discussion either, but on modeling part, I can make them quite quickly, even if UVs are needed for them. I still have what I made for the ships, so only a few need to be made right now.

About management/coding: I can't really tell from my viewpoint, how much management is needed so I can't really on that part. And doing only management sounds quite not fun for sure. How much work goes to managing contributions, PRs and stuff?
I remember that there was a monthly release cycle half or so year before. Maybe bringing back such a thing could have benefits, like feature freeze or something, some wrapping up, then do a release, and go back to actual coding? (This seems to be working for the blender developers, but they do have management).

I'm obviously talking naively from the edge of the game court, and I don't really know how was the previous release method working. But some kind of release cycle may work if it's not based on a given fixed period of time, but on some more loose way. Like somebody shouts for release when something's ready, the stuff could be checked and reviewed then, and released if it's nice (say, two other developers check it and say OK or NO and telling why), and then going back to the actual fun stuff. (Ideally nobody should need to fix the work others, I try to keep this attitude with the models I make)
Or if a certain amount of things come in, and got two quick check OKs from the others, then there could be some kind of feature freeze, checking the new things throughout (and all hands who can do that, are doing management basically), do a release. Maybe with a cool-down period for each developer, so nobody spams the others with PRs, so they can concentrate on their things. (Not that I could help too much with the coding aspects though, unfortunately).
Does this even makes sense? :)

And maybe developers could do a very bit of blog-ing or something, just putting down a few words, what they are working on (nothing as fancy as a full fledged, or even public as a devlog though), so the others could peek over their shoulder. (Like when I put up the WIP for my ships, or the self notes gist I posted for cockpits, and I plan to do it for some other stuff I want to do, like touching up the buildings a bit for example).
jpab
Posts: 77
Joined: Thu Jul 18, 2013 12:30 pm
Location: UK

Re: Existing PR's

Post by jpab »

(I'm sorry, this post is going to jump around quite a bit -- I don't have time to structure it properly).

I think there are several reasons that PRs sit in the queue for ages, but fundamentally they all come down to looking at the PR and being hit by that sinking feeling that "this is a mess and it's going to be difficult to deal with". This is a feeling that puts me off reviewing things and makes reviewing just one non-trivial PR feel like a pretty big effort. Hell, just looking at the PR queue can be draining when you know that a) they're not in a state that you want to just merge immediately, b) the problems aren't just clear bugs that you can just throw back to the submitter with a quick two sentence description/explanation.
I can step down from project stuff, ignore the PR queue and just work on code. There's nobody that really wants to pick up all the stuff I do.
If you're fed up with the PR queue and want to stop dealing with it, then I won't try to convince you to keep at it. You've always been clear with me that Pioneer is for fun, and none of us have an obligation to work on things we don't want to, and I would like to say the same thing to you. Pioneer is for fun, and you do not have an obligation to deal with pull requests if that's not something you want to do, and that is true regardless of any impact on how fast the project progresses.
I can stop writing code and just do project stuff. I've tried that before on a previous project. I burned out and it took a year before I wanted to touch a computer outside of work. I'm rather keen to avoid that.
I don't want you to burn out either, so unless you can see something seriously different about Pioneer then I would advise against this.
I can open git access to everyone (and here's some reading on a model that might work). Same danger.
I wouldn't try to block this if you think it's worth a shot, but I suspect it wouldn't work well for us. There are practical issues of level of confidence with git and level of programming experience that would put a lot of people off merging things, or could result in confusion or other problems.
robn wrote:I can lower my standards, mostly just merging anything that comes along. I worry that the lack of direction will kill is (and we do have some direction, believe it or not). I also worry that it'll add more mess to the codebase. That's going to feel too much like hard work being undone, and I won't be able to watch it for long.
For what it's worth, I think this is the most practical option of the ones you've listed.

I said above that PRs sit around for ages because they feel hard to deal with. This is a psychological problem, not a technical one. We need to find ways to make processing the PR queue easier, not in terms of technical effort required, but in terms of the mental and emotional energy required.

Perhaps it will help to talk about what types of problems PRs can have, and make some guidelines about how to deal with them.
  • Bugs, logical errors, mathematical errors, typos (any simple, isolated code error). These are easy. If we (or someone else) finds a bug during testing or by looking at the code, either fix it or tell the submitter to fix it. It's easy because everyone agrees that bugs need to be fixed.
  • Uncertainty about some code logic or behaviour ("Is this maths right?" "Did the submitter intend this weird behaviour?"). These are easy: Ask the submitter (obviously).
  • Code style errors (braces, whitespace, naming conventions, etc). Same as bugs: fix it, or tell the submitter fix it. Not everyone agrees about the importance of code style, or perhaps even what style we should be using (particularly since the existing codebase is not totally consistent), but style problems are easy enough to fix that if someone wants to argue about them instead of just fixing it and moving on, then that person probably isn't worth dealing with. If there is some aspect of style not covered by the code style guide on the wiki, amend the guide and tell the submitter to fix their style. Yes, style is subjective, and it's not the submitter's fault that they didn't use the "correct" style, since it may not have been clear what was the "correct" style when they made the PR. I don't care -- the time and energy to discuss it costs more than just fixing the code.
  • Messy git history. Fix during merge. I've given up on asking anyone to clean up their own git history -- a lot of our contributors just don't have enough confidence with git to do it right anyway, and the ones that do tend not to submit messy histories in the first place. Therefore, it's not worth anyone's energy discussing it. Whoever is processing the PR can rebase, split and squash commits as necessary.
  • Game design uncertainty ("Do we want this feature?", "Do we want it to behave this way?"). Close PR, tell submitter to start a thread on the dev forum to discuss it. Tell submitter that it is their responsibility to make sure that discussion reaches a conclusion -- if the discussion actually does reach a conclusion (I'll believe that when I see it!) and the conclusion is that we do want the feature, then the the PR can be reopened or the submitter can resubmit if there were design changes needed.
  • Code structure problems ("We want the feature, but not implemented like this"). I think this is by far the hardest one (a lot of the above we already deal with as I've described). Structural problems can take a significant effort to fix, they make future code maintenance harder, they're problematic all round. This is the one that that sucks PRs into a timeless void, and this is the one where robn and I need to lower our standards and be more willing to merge things that we know will need to be ripped out or totally rewritten later.
I'm going to ramble about the last one for a bit, because it's the hardest.

My instinct is to try to filter out things of questionable quality or desirability at the review stage, to try to keep master on a steady path of improvement and avoid adding code that I believe we will have to totally rewrite. Even more strongly my gut tells me that it's bad to add Lua API or content format features if there's a serious chance of that interface having to be removed or significantly changed later. But, all the evidence I can see suggests that that just isn't working -- Pioneer is improving, but code quality is certainly not following a monotonic path upwards.

My experience has been that trying to fix structural problems by pointing them out to submitters is very hit-and-miss (emphasis on "miss"). It's also my experience that discussions in PR comments about problems of how things are implemented are irritating and energy-sapping for me and probably for everyone concerned. It may be defeatist of me, but the only solution I can see is to minimise that type of discussion. If there's a structural problem, robn or I should fix it when we merge, or if that will take too much time, then we should add some fixme notes to the code and merge it despite its flaws.

The only options I see are: throw it back to the submitter telling them the structure isn't right (which doesn't work), close it (which is just throwing it back more forcefully, and I don't see any reason it would work any better), or merge it and accept that it means more stuff that needs to be rewritten in the future.

This isn't a good solution. It's not a solution I like. I don't think it'll be easy (merging bad code feels bad). But I think the alternatives might be worse.
FluffyFreak
Posts: 1343
Joined: Tue Jul 02, 2013 1:49 pm
Location: Beeston, Nottinghamshire, GB
Contact:

Re: Existing PR's

Post by FluffyFreak »

When I suggest that perhaps the level of coding ability required is a bit high what I'm trying to say politely is that you guys have higher standards than Crytek, Epic or Sony.

That doesn't mean that you should let any old random shit into the engine, or that you should bear the brunt of restructuring code or doing 101 other possible things. I worry mostly that you've set the bar so high that there's effectively almost just two people who can commit new features: you two.

That's kind of ok in that it does guarantee a high quality of code but it does also mean that no-one else is good enough - no that's not the right way of putting it, sorry my brain is tired.
It feels like the code quality is more important than either features or the game itself, to the extent that it'd be preferable to have the game development on hold because a feature didn't tick *all* of the boxes.

I try to follow the coding style (http://pioneerwiki.com/wiki/Code_style), (which is tricky because I have my own, still keep doing bits of Cryteks by mistake, and have to use Epic's everyday!), but I'm also suggesting that it's... a bit of a hindrance in some ways: http://scientificninja.com/blog/write-games-not-engines

What's the goal? Writing really pretty code or making a game? Because making a game in a realistic timeframe will need some compromises and the game design (http://pioneerwiki.com/wiki/Design_Scope) and roadmap (http://pioneerwiki.com/wiki/Roadmap) suggest that we want to get things done.

One of the things that attracted me to Pioneer was that it was a bit rough but it gets the job done, it works and is playable, and fun, and going somewhere.

I've worked with architecture astronauts before, they'd create massively complex pieces of code that had ludicrous coding standards, template madness, bonkers structural forms, cutting edge shit... the games never shipped or they required massive teams (hundreds of people) and took years (9+) but would never work right and worse than that were impossible for anyone but the very very very best to fix or maintain.
On the other hand I've worked with people who I've caught littering my code with goto calls and caused infinite loops which drove me mad and I would not welcome that situation again... on the other hand they actually finished their games and shipped them to critical acclaim with just a handful of developers.

Hopefully it should be possible to find a happy medium :) (no more gotos, please no more gotos!) I'm not after a free-for-all because you're quite right that it'd be a mess, and stupid of us. I like jpab's suggestions and wouldn't mind getting things bounced back, for me the problem is that I can't move forward without something to go on. If a PR looks like a nightmare then I need to know so I can fix things, if it sits there for 3 months without comment/cancelling/merging then eventually I forget how or what I wrote.

Andy
FluffyFreak
Posts: 1343
Joined: Tue Jul 02, 2013 1:49 pm
Location: Beeston, Nottinghamshire, GB
Contact:

Re: Existing PR's

Post by FluffyFreak »

robn wrote:
I started talking to nozmajner about the desirability of this, but I don't recall if we reached a conclusion. I'm guessing not because I didn't write it down.
It's a pretty popular idea, I liked it enough to code it twice ;)
robn wrote:
The mention of a frame glitch put this straight onto my "to check later" list. Which isn't really a list, mostly just a sigh that there's too much to do.
No worries, you mentioned in one PR that you needed a list :) here it is!
I think that I fixed the one frame glitch but I am fuzzy on the whole thing now.
robn wrote:
I don't disagree with the concept, or the need for it. It was mostly that it was on the old UI that was the problem.
If I could figure out how to port it to the new GUI system it wouldn't be a problem?
robn
Posts: 302
Joined: Mon Jul 01, 2013 1:11 am
Location: Melbourne, Australia

Re: Existing PR's

Post by robn »

Thanks all for your honesty. I really appreciate it. I've learned nothing new, but its nice to have it all out there.

Its been clear for a while that something has to give, and I think its now time, because it can't go on like this. I'm still working through some of the details with jpab, but the basic theme is to open the doors wide and let everyone in. That is, hand out commit bits.

I'll be posting some stuff on the wiki and to the main forum as soon as I can. I'm pretty sure within the next two days.

Fluffyfreak, I'll reply to your PRs individually.
Locked