Feedback For a General overview.

Hello world...

New to unity and been spending a little time learning.

I am making some progress on an idea, however before getting to in-depth, or better diving deep in to the code part of it all, I would truly appreciate if any one who has the time to take a boo at the following scripts, I am looking for feedback and input on how better to do whats being done.

Please take a moment to review the following "Work In Progress" scripts while providing samples, and a explanation as per why one method may be better then the other, my Goal here is efficiency and proficiency = stability and speed.

Brief: The following Scripts all connect to a Main Systems Host, named "Mazzy_Systems", Mazzy_Systems resides on a EmptyGameObject with the follow scripts as children of it, these are 3 /12 primary Parts of this system.

I appetite your time, also, do not hesitate to ask if I need explain how things are intended to work.

PS: keep in mind, these are a work in progress and you may notice somethings are commented out.. for now

BingoZee

 

Reviewing sheets of code isn't generally helpful, especially when it's just a bunch of random files out of context. Remember in Unity that the code is a tiny tiny tiny part. FAR more important is how the scenes, prefabs and other assets are set up.

More generally, reviewing code in general isn't really a thing. I know you want it to be, and it certainly might provide value, but if you're new to coding, you won't possess the context required to even understand feedback you get.

A far MORE valuable way is for you to do things a lot of different ways while at the same time understanding the pros / cons of each way.

One good way to evaluate your own software is to consider how hard it was to make and understand, how many bugs you encountered while making it, how reliably and how effectively it solves your actual problems, and also how easy it is to change in the future, to add features to, to upgrade, etc.

 

I am wondering why your even bothered to comment as None of what you had provided was useful or of any value at all.

I am asking people to Help Streamline what is there, if they are willing which clearly you where and admit are not willing to do, again, Why comment at all....

Lastly, I think if you did, you would realize how the scenes, prefabs and other asset set ups are irrelevant.

PS: "Reviewing code in general isn't really a thing", then what the hell is this forum for ???, I am not that new.... I am however more a designer then a coder

Take care, I am seeking more creative and constructive, useful feedback, again from those willing.

 

Hi Bingo,
I believe Kurt took the time to answer your question because he was trying to help you out. I'd advise responding in a more humble manner when people take the effort and time to try and help you.

I agree with Kurt on this one after downloading the files and taking a quick look.
No one can know what it is you are trying to do because you only provided a tiny fragment of the bigger picture, which is like Kurt said, the project, the assets and the way they are all used together.

Asking people to look at scripts with over 1000 lines of code with no clear question on what to fix isn't going to lead to what you want.
The more abstract the question the more abstract the answer will be.
It's your job to find inefficient code or bottlenecks, isolate it and then share that with people and ask for help, I don't think these forums are the place for code reviews spanning multiple scripts with little to no context.

Tip for the future. Don't expect users to download all your scripts and then rebuild your project.
Clean up everything, create a project and make a download link available. Then people can look at the bigger picture.
Or at least upload the scripts to github and provide specific questions like
"I have this for loop on lines 96 to 112 in this script, can anyone tell me if there's room for improvement?"

All the best with your journey.

 

The purpose of Code Reviews is to figure out if there is a problem, usually what we do on this forum is just solve problems people know already exist. While it's not an incorrect place to put it, it's unlikely anyone will do a Code Review for you of this size.

You gotta remember, most people here are just a bunch of strangers putting in volunteer work on the internet. If you're really intent on getting feedback on your work, you should split it up into more bite-sized chunks that people can easily read, put it in the post using Code-Tags, and probably a different thread for each chunk. I wouldn't even go past 50 lines of code.

 

I had a quick look over your code yesterday However without any documentation or at least a rough explanation it's almost impossible to tell what those 4 script files and those dozens of classes actually do or should do. We can only partially guess what it's about.

There are countless of unrelated fragments jumbled together. Just for example, what is this actually good for:

Code (CSharp):

1. public FaceEfx_Assests[] FaceEfxAssests;

The class as well as this field is not used anywhere in those 4 files, yet it's declared in the class. Your code massively violates the DRY (Don't Repeat Yourself) principle. This is not a strict rule. In certain cases it's completely fine to have something two or three times repeated if it's just a small portion. However you have walls of repeated code.

The whole raycasting part / class performs a similar raycast i think 6 times in a row with a page of code per raycast. You included that "MazzyAudioControl" class which doesn't seem to control much as it's mainly a collection of predefined data. The usage of the class is only to be assigned to a class instances which are not among the stuff you shared, specifically that Pad-, Pot-, and PieceAlive classes.

So to me it's not really clear what you actually want from us. We have almost no way to understand what this code is supposed to do since you provided no explanation and the code you shared is missing a lot. This is a support forum where developers come when they have a problem and need help. As Kurt said, we don't do code reviews. What exactly should be the outcome of our review?

I can say that you clearly have too much duplication and tight coupling between classes. Also your classes seems to be responsible for too many things at once so some could almost be classified as god-objects. I suspect there's a good chance that most of this could be solved way simpler, though to actually suggest an alternative you have to understand the purpose of the code first.

 

I'd just like to add that usually people complain all the time that nobody answers their post. This happened much more frequent on UA because there are much less people actually writing answers. So I'm a bit baffeled that you complain about getting a response. I do understand that it may not be what you wanted to hear, however it's not clear what you actually want

I already spend way too much time on this, but let me quote some points of the code of conduct.

 

I stand firmly by my original sentence:

We don't even touch code reviews on my team. Code reviews in Unity are essentially useless. You may have the prettiest tightest sweetest code in the whole world... but if you forgot to drag the prefab in... *BOOM* So much for your pretty code. Now it is "Why do I have a null reference!?"

On our team we do feature reviews, which includes the author(s) demoing the feature (we use Discord and screenshare) while explaining all the prefabs, connections, purpose and point.

We call these PRPRs, or Peer-Reviewed Pull Requests.

They are INCREDIBLY valuable. We have gotten so proficient with PRPRs that the entire team can be present for a ten minute review of something two of the team members have been working on for an entire week, and when everybody is thumbs-up, we merge it into develop / master and off we go.

This process adds value, adds perspective, adds visibility, and provides a way for everybody to chime in with their own area of knowledge if necessary. And it actually catches legitimate errors, such as "Oh I forgot to put a collider on the head of the one enemy..."

 

Like a lot of others here, I can't really tell what any of this code is supposed to do, if anything. My gut is telling me that you want to delete as much of this as you possibly can. Just off-hand it looks like there's a lot of pointless encapsulation in there. Maybe I'm wrong, though.

Edit: one simple thing you can easily fix:
There's lots of places in your code, where you are doing pointless checks like this:

Code (csharp):

1.  
2.  if (MazzyCam.depthTextureMode != DepthTextureMode.Depth)
3.             {
4.                 MazzyCam.depthTextureMode = DepthTextureMode.Depth;
5.             }
6.  

When you could get the exact same result without checking. just replace it with this:

Code (csharp):

1.  
2. MazzyCam.depthTextureMode = DepthTextureMode.Depth;
3.  

 

When I first posted the files, I knew it would be hard or as mentioned impossible to get a better idea of what is happening unless I could provide a Project file that includes a sample scene and as mentioned above, the Prefabs, objects and so on.

For those who had found the time to take a peek, I would like to thank you even if for a simple answer such as kdgalla gave, although simple, will effect a lot.. i.e. pointing out the pointless testing.. On that note, I am aware of these kind of things and as I develop the code, I some times tend to use the worst methods before finding the best, a kind of get it working, then tune it up, and make it look pretty latter.

To that point, from the time I had posted those files, till just a moment ago I have nearly completely rewritten them, but think to post these updated scripts along with a sample project, as mentioned its hard to get help otherwise as I do appreciate the feed back..

PS: I wonder, Most if not all of my code is 200+ lines of code, I can not imagine how to do it in less, even while trying to eliminate all the pointless tests, and so, Hope to get some feed back and ideas of how to optimize as much as possible.

As for braking the code in to smaller chunks, I have been from the beginning and ended up with what I am calling Apps, each App adds a new feature to the game.

The game itself is much like a Checkers, Chess, War, Snakes and ladders, that includes Card draws and dice rolling, in other words a very advance form of chess with up to 4 two player teams played in photo realistic and fantastical environments, complete with weather, day and night, bla bla bla.

"Mazzy Chess." & "Mazzy Masters"

these are the names of those chunks.
Mazzy_SystemsMain
Mazzy_RayCast
Mazzy_AudioControl
Mazzy_MotionControl

Mazzy_Bridge
Mazzy_PieceAlive
Mazzy_PadAlive
Mazzy_PotAlive

Mazzy_CardDraw_App
Mazzy_ColorMod_App
Mazzy_CountDown_App
Mazzy_InfoGizmo_App
Mazzy_MainMenu_App
Mazzy_MiniMap_App
Mazzy_MusicPlayer_App
Mazzy_PieceFinder_App
Mazzy_PieceInfo_App
Mazzy_PlayerProfile_App
Mazzy_PlayTimer_App
Mazzy_Roller_App
Mazzy_Spawner_App
Mazzy_Status_App
Mazzy_TargetInfo_App

Hence my only posting 4, imagine if I posted them all... I expect there will be a few hundred when done Anyway, thanks again and I will not bother updating this post until I have a decent sample project to offer.

PS: Cheers.

 

Oh there's your problem! You're trying to optimize frivolously! That's the actual problem here... no wonder you're so confused by the responses.

DO NOT OPTIMIZE "JUST BECAUSE..." If you don't have a problem, DO NOT OPTIMIZE!

If you DO have a problem, there is only ONE way to find out. Always start by using the profiler:

Window -> Analysis -> Profiler

Failure to use the profiler first means you're just guessing, making a mess of your code for no good reason.

Not only that but performance on platform A will likely be completely different than platform B. Test on the platform(s) that you care about, and test to the extent that it is worth your effort, and no more.

https://forum.unity.com/threads/is-...ng-square-roots-in-2021.1111063/#post-7148770

Remember that optimized code is ALWAYS harder to work with and more brittle, making subsequent feature development difficult or impossible, or incurring massive technical debt on future development.

Notes on optimizing UnityEngine.UI setups:

https://forum.unity.com/threads/how...form-data-into-an-array.1134520/#post-7289413

At a minimum you want to clearly understand what performance issues you are having:

- running too slowly?
- loading too slowly?
- using too much runtime memory?
- final bundle too large?
- too much network traffic?
- something else?

If you are unable to engage the profiler, then your next solution is gross guessing changes, such as "reimport all textures as 32x32 tiny textures" or "replace some complex 3D objects with cubes/capsules" to try and figure out what is bogging you down.

Each experiment you do may give you intel about what is causing the performance issue that you identified. More importantly let you eliminate candidates for optimization. For instance if you swap out your biggest textures with 32x32 stamps and you STILL have a problem, you may be able to eliminate textures as an issue and move onto something else.

This sort of speculative optimization assumes you're properly using source control so it takes one click to revert to the way your project was before if there is no improvement, while carefully making notes about what you have tried and more importantly what results it has had.

 

Thank you Kurt, I was just starting to look in to the Window -> Analysis -> Profiler...but I have only one platform in mind as the target, PC.

My goal, is to see how-much I can get out of my head in to code within a year, In my head its all there, but as we all know from the head to the hands is the hardest part.

Mazzy is a project I thought about for 4+years before ever opening unity, I tried Unreal, but I stuck with unity, its not a make as you go project, its a how can I get all of this out of my head as it is in my head thing!

 

For me, this highlights the kinds of specific Q's that work here and how people interpret them. We get Q's about Unity specific features -- like asking about an awkward raycast, which could be fixed with layers and so on. Similar are Q's about working with Unity -- like how to get global data and state among scenes, or handle multiple colliders on the same object. We often get generic programming Q's which are really about tutoring -- they say they're barely self-taught, have 6 pages of similar code, and clearly want to hear about arrays and functions.

The problems with "code review" are it's too broad. People have to guess which category it's in -- and we automatically assume it will be one. Yours could be about raycasting, or maybe you'd love to be told about

List<Transform>

, or you want to know more about the Unity way, or want a generic microsoft-centric C# code review, or a pragmatic game-coder-style code review. Basically, in the Stack Overflow C++ area, "code review" has a specific meaning, but here it means lots of things of which traditional code review is the least likely.

 

My only suggestion to get it out of your head faster is to:

- spend more time getting it out of your head

- spend LESS time worrying about what other people think about it

I hope you are soon able to get it all out of your head.

 

You've been making use of nested classes but you haven't been taking them as far as you could have.

Spoiler: Original Apps

Code (csharp):

1. #region Apps
2. // --------------APPS
3. [System.Serializable]
4. public class Apps
5. {
6.     [Space]
7.     [Header("CardDraw Connection")]
8.     public Mazzy_CardDraw_App _Mazzy_CardDraw_App = null;
9.     [Header("CardDraw Status")]
10.     public bool _IsCardDrawAppLoaded = false;
11.     public bool _IsCardDrawAppReady = false;
12.     public bool _CardDrawAppOn = false;
13.     [Space] public bool _IsCardDrawAppDebugOn = false;
14.  
15.     // rest of them
16. }
17. public Apps _Apps;
18. #endregion

Spoiler: Improved Apps

Code (csharp):

1. #region Apps
2. [System.Serializable]
3. public class AppStatus
4. {
5.     public bool _Loaded = false;
6.     public bool _Ready = false;
7.     public bool _On = false;
8.  
9.     [Space]
10.     public bool _DebugOn = false;
11. }
12.  
13. [System.Serializable]
14. public class Apps
15. {
16.     [Space]
17.     [Header("CardDraw Connection")]
18.     public Mazzy_CardDraw_App _Mazzy_CardDraw_App = null;
19.     [Header("CardDraw Status")]
20.     public AppStatus _Mazzy_CardDraw_Status;
21.  
22.     // rest of them
23. }
24. #endregion

 

Ryiah This is the kind of Feedback I was seeking! thank you so much!. I really hate having to Repeat code, and as mentioned, even the smallest tips, I am sure can and will make a major difference over the development.

I would also mention, Of course I do not expect any one to go over the files I upload line for line, but to spot and point out these things, for me, Very helpful!

 

lol, easier said then done, but I have to admit, I am afforded time and on a typical day can easily punch away at it for 14 to 18 hrs on and off throughout the day, or so have been for over the last two months now, mostly testing and learning, I cant say I am even in to the coding part as of yet, however, as mentioned I am not that new, I was a programmer back in the dos, Basic, pascal and C+ days, however, that was also some time ago, and if we don't use it, we lose it, and as a consequence I am ReLearning, hell I use to develop windows applications, and POS systems , but then I explored other things.

Anyway, I was a retired father of 5 at 35, and so no rush, just steady as it flows for me. I am having fun so far...

I hope, when I get to uploading a Sample, you might take the time to give it a go, till then, take care.

PS: "spend LESS time worrying about what other people think about it" Thankfully, for myself, I don't care for what people think, I care only for what they know, but I guess that settles the "Getting to know you part", I will make it a point from here on to keep it about Scripting.

Nice to meet you all... Hope to get your feedback again next time round...

 

Owen-Reynolds.

I think your referring to the following file, if so, I would appreciate any tips on how to optimize.

How it works: On mouse click, Casts a Ray, in all directions, Up, Down, Left, Right, etc.. If there is a collision it sets the game-object to a list, It also Sets primary Variables on each App if the App is present, such as the RayCastTarget, w/ most "Apps" require.

I am asking, is there a way to optimize what is happening here ?

Please keep in mind, "All" of the Mazzy scripts are continuously evolving but to incorporate better methods, or so I hope, I am not sure its relevance, but during game time there will be many active objects, and so optimization is a critical aspect of what I aim to achieve here.

Again, thank you for your feedback, if your have any input or suggests of how to optimize this file please do not hesitate.

thank you Owen-Reynolds.

 

Just glancing at the raycast file it seems like basic class and array theory would be useful -- like if it got deleted and you were writing it again.

Each of the 6 directions has a button, and, ummm, more stuff. Traditional 1970's design says to put all the stats for 1 side into a class. Say it's called

CubeSide

. That makes it easier to declare everything for a side and to pass a side around. Simple OOP -- still 1970 -- says you may as well make a reset() member function, or setActive() and so on.

The next trick from the 1970's is putting all 6 into an array. It seems as if 6 directions are too different for that, but a game trick is to put the direction in the class, like

Sides[0].castDir=Vector3.up;

and so on. Another generic trick -- have each one know it's button name:

Sides[0].name="Top";

. Now you can have

Sides[i].showName(true)

copy it's saved button name into the button text.

But this requires some experience. If you're just using Sides[2] instead of LeftSide, there's no gain. You've got to be able to write a loop that treats them all the same, with the different parts (raycast dir, button name) handled by class variables instead of weird IF's.

 

Thank you Owen, I was born in the 70's I understand what your trying to convey.

The RayCast, Casts A Single Ray from all Side of a Cubed object, top, bottom, etc.. If any of the Rays collide with a GameObject, that object is then saved to a Variable, be it HitRt, HitLf, etc..

The Variable is not used by RayCast itself, but pulled by other scripts, along with the Original Object casting the rays, I did included some Gui Elements, that are either enabled or disabled depending on the Hit returns, and that Pass these Variables off to a "APP", a different script that then shows the GameObjects detected.

Anyway, I have made some major changes to the System, Where I have now integrated the Mazzy - RayCast, Audio, Motion scripts. Its faster, but I am noticing a leg in the camera movement parts, I am also aware I have a few conflicting Key presses during Edit / Debug mode, However, I am working at Turning this particular file up as I intend to base the other scripts off of this primary script, finally, this Script acts as a switch board and resource for the other scripts, It dose not control game objects other then simple movement, that will only be functional during Edit mode, Where as in Play mode, I am developing Mechanic Scripts, Nav managers, and some simple Ai logic, along with Editor scripts that will definitely make things easier to manage.

Just thought to update for any following the post, in addition, I have included the currently developing Mazzy_Systems script for any who may have the time, and care to provide any input, advice, or even examples of how better something can be done, Please do not hesitate.

Cheers.

PS: Just putting it out there, Interested in learning more about the Mazzy project, let me know, I would be happy to provide what ever information I can.

Everything is Divided by #region making it easier to read.

 

One more thing I noticed (since made the same mistake on one of my products), there's no need to put

Mazzy_

in front of everything. It's a great trick to avoid clashes with identifiers from other projects, but namespaces do it better -- they were invented for that. Just wrap everything in

namespace Mazzy;

(even things in different files). Now everything in your files is named

Mazzy.SystemsMain

, and so on. Clashes are successfully avoided with some other SystemsMain. But your own code can leave out

Mazzy_

. Seems trivial, but not getting blasted with all of those MAZZY's makes reading it much easier.