Thread Rating:
  • 0 Vote(s) - 0 Average
  • 1
  • 2
  • 3
  • 4
  • 5
Offering help.
#1
Veteran Unity dev here. I mostly code but I'm something of a generalist. Starflight is my favourite game, so I was overjoyed to find this. I've been looking over the repository, and besides a few nitpicks the codebase looks very clean and approachable. Working on my own KS, but I think I can find some time to contribute. What areas do you want help with specifically?
Reply
#2
Hmm, good question. I'll figure something out and let you know. Very curious about what your nitpicks are. This is my first Unity project so I'm sure there are tons of things that I didn't do the "unity" way because I've been pretty much winging it. I've run into lots of barriers trying to do things in Unity that I've done in other game engines professionally.
Reply
#3
Well you could have fooled me, Marvin! Things generally look really good, and yeah, I've seen some experienced programmers without Unity experience do things... in ways that might make sense elsewhere but very much clash with the Unity SOP, but at first glance I didn't think you were one of those. So... congrats! Although as I'm writing this post and finding a few more things, it is becoming a bit more obvious (Although you stating so much probably creates a bit of a confirmation bias) Wink

That said, UI and input seems a bit odd. For example, querying the input controller to advance the state of a UI script as opposed to just using the built-in event system. Every button having its own class, and not using the built in Button component or something deriving from it, seems a bit unorthodox too. Namely because you're increasing code verbosity while only retaining a subset of the functionality that regular Unity buttons have.

One nitpick is making fields public for the purpose of exposing them to the inspector. That's what the [SerializeField] attribute is for, but I'm guessing you missed that and the knowledge it exists will come as a relief (thanks, official Unity tutorials for always making fields public!).

Next is perhaps debatable, but turning MonoBehaviours into singletons can be a bit evil, since if you're trying to retrieve the instance before its Awake() has been called it returns a null reference. Worse yet, unless script execution order is explicitly set the order can vary between in-build and in-editor, which can be a huge pain to debug. I don't think there's any kind of general standard here, but my practice is to create a MonoBehaviour gatekeeper class that contains a static reference to every persistent would-be-singleton MonoBehaviour, which is then set to initialise before anything else in the script execution order so it can populate those static references before anything else happens.

And since I'm working through the codebase as I write this out, I'm also noticing a bit of lax encapsulation here and there. For example, DoorNameController.GetDoorName returns the text property of a TMPro component, which is then used in AstronautController to check which panel to open. This is, of course, not great. So if some schmuck comes along and decides that, say, those names would look better in ALLCAPS and changes the value in the inspector he'll actually break the program -- which should never happen from editing the contents of a text component. Rather, a separate id value or just a reference on the DoorController itself to the panel it manipulates would be much preferable.

Last is a total nitpick, and again debatable, but there are a few areas that are somewhat verbose versus a LINQ expression. Eg. the body of GameData.FindElementId can be simplified to a single simple line:

Code:
return m_elementList.FirstOrDefault(x => x.m_name == name)?.m_id ?? -1;

Aside from the benefits of expressing the same operation in fewer lines and characters, this is generally more readable. You can speak what's happening left to right just like a sentence. Of course, this is assuming the reader is mostly comfortable with dot notation and null coalescing (which a C# programmer should be), but there's an argument to be made for keeping things approachable for those who don't. And in performance critical code, you generally want to profile LINQ versus traditional loops too.

And I'm sure I'll find a few more things as I work my way through the codebase. But, I feel I should restate again how easy I'm finding it to follow everything in case I'm coming off as a bit too complain-y. That in itself is quite the triumph!

Do you have a Skype or Discord handle you wouldn't mind sharing with me? Would be awesome to be able to chat a bit before I touch anything so that we aren't at risk of walking on eachother's toes and all that.
Reply
#4
All good stuff! I'll be posting a more detailed response later. I'm in Vegas right now participating in a bowling tournament. So far I placed 2nd in singles and also 2nd in doubles as well. This is out of 80 players. Tomorrow will be the team matches.
Reply
#5
That said, UI and input seems a bit odd. For example, querying the input controller to advance the state of a UI script as opposed to just using the built-in event system. Every button having its own class, and not using the built in Button component or something deriving from it, seems a bit unorthodox too. Namely because you're increasing code verbosity while only retaining a subset of the functionality that regular Unity buttons have.

The UI in Starport was literally the very first thing I did in Unity.  So yeah, if I did that again today I'd probably do things a lot differently.  I remember struggling with the built-in event system because I wanted to shut off mouse support, but there didn't seem to be a reliable way to do that.  It's still broken-ish in that respect.  As for the spaceflight ship buttons, each button has it's own class because the actual button function is self-contained within it.  Take a look at the maneuver button for a good example.  It lets me easily swap out the button UI and functionality together as the player navigates through the different ship roles.  Some of those buttons do nothing more than display a "not implemented yet" message - just waiting for the function to be willed into existence.

One nitpick is making fields public for the purpose of exposing them to the inspector. That's what the [SerializeField] attribute is for, but I'm guessing you missed that and the knowledge it exists will come as a relief (thanks, official Unity tutorials for always making fields public!).

Aha.  Yeah, much of what I was taught about Unity came from the Unity tutorials.  It doesn't help that the Unity documentation sucks really bad.  Many attributes have little or no explanation of what they do, or how it works, or even what units the values are in.  The shader documentation is all over the place.  I cry a little bit on the inside whenever I have to crack open the Unity docs.

Next is perhaps debatable, but turning MonoBehaviours into singletons can be a bit evil, since if you're trying to retrieve the instance before its Awake() has been called it returns a null reference. Worse yet, unless script execution order is explicitly set the order can vary between in-build and in-editor, which can be a huge pain to debug. I don't think there's any kind of general standard here, but my practice is to create a MonoBehaviour gatekeeper class that contains a static reference to every persistent would-be-singleton MonoBehaviour, which is then set to initialise before anything else in the script execution order so it can populate those static references before anything else happens.

Yeah the MonoBehavior object life cycle is a bit frustrating - about halfway through the project I completely gave up on using Awake(), Start(), etc. and am now doing just my own Init() at the right time.  Most of the stuff is still using Awake() etc but all of the newer stuff (terrain grid, etc.) don't.

And since I'm working through the codebase as I write this out, I'm also noticing a bit of lax encapsulation here and there. For example, DoorNameController.GetDoorName returns the text property of a TMPro component, which is then used in AstronautController to check which panel to open. This is, of course, not great. So if some schmuck comes along and decides that, say, those names would look better in ALLCAPS and changes the value in the inspector he'll actually break the program -- which should never happen from editing the contents of a text component. Rather, a separate id value or just a reference on the DoorController itself to the panel it manipulates would be much preferable. 

Yeah - the starport UI was the first bite of Unity I did, and so you see stuff like that.  :-)  A slightly more knowledgeable person would go into the TMPro component and switch on the all-caps render modifier instead of going into the code and making the strings all caps, but you are of course right.  I think at the time I had no idea how different MonoBehavior components should be communicating with each other, and I figured it out much later.

Last is a total nitpick, and again debatable, but there are a few areas that are somewhat verbose versus a LINQ expression. Eg. the body of GameData.FindElementId can be simplified to a single simple line:

Yeah, there's a couple of reasons for that - (1) You've probably noticed the insane amount of comments in the code.  I think I put a comment on basically every line of C# code.  I don't actually put in that many comments professionally.  :-)  The reason for the amount of comments is because I am hoping my two 16-year-old boys will become interested in this and learn to code through this SF project eventually.  So yeah LINQ would probably complicate things for a beginner.  And (2) my last experience with C# prior to this project was writing tools (level editor, material editor, texture converters, etc.) around 2009.  I don't remember LINQ being around back then.  I do use dot notation and null coalescing in other languages (PHP, etc.)  The languages I use professionally right now are PHP, JS, Java, and Obj C.  Back when I worked for Rainbow Studios on Cars and Deadly Creatures and so on, we would never use C# for the actual game - it was all C++ and a bit of assembly, and scripting was done using Lua.  Waaaay back when I was working for Bethesda Softworks on Redguard, etc... it was mainly C and a lot of assembly.  Back then there was no such thing as hardware 3D acceleration... it was all software rasterization.  Around 1996, I worked on some of the very first 3D accelerated stuff (3Dfx / Glide).  Good times.  :-)

There are a lot of gotchas with using Unity.  For example, I recently found out the hard way you cannot populate the world and use collision detection during the population loop because Unity doesn't update physics until the next tick... so I had to roll my own temporary mini-collision grid for populating the rocks and trees on planets to prevent distance checks from being big O(n^2).  No support for transparent objects receiving shadows (come on, I did that in my Chasers game engine back in 2006).  Poor support for multithreading (especially when dynamically generating GPU assets).  It is the little things like that make me want to scream.  :-)

But overall I do like using Unity because it does allow for very rapid development, as long as I accept the limitations and bugs.  I've built about 5 game engines for different companies and am pretty much done with that, although I do really miss some of the stuff they could do.  I am looking forward to the HDRP stuff but the last few times I tried it, Unity choked and died on some very simple shaders I tried to build in the shader graph... so that's not ready yet.

Do you have a Skype or Discord handle you wouldn't mind sharing with me? Would be awesome to be able to chat a bit before I touch anything so that we aren't at risk of walking on eachother's toes and all that.

Hmm - I can invite you to our facebook messenger group - we have a little private one that we've been using.  Send me an email so I have your email address, and I'll invite you.
Reply


Forum Jump:


Users browsing this thread: 1 Guest(s)