Search Unity

Do you mind to improve this code?

Discussion in 'Getting Started' started by jjgarcianorway, Dec 22, 2016.

  1. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Hello all,

    I'm coming from Corona (Lua) and this is completely new for me so I'm sure I'm doing tons of things wrong...

    I know that there are tons of tutorials about C# but... practicing is the best way for learning (I think) :)

    What I have now is:

    Editor:

    2 Prefabs (Icosphere with Material and a Text Mesh)

    What I want:

    Create X number of Icospheres with same number of text meshes and be able to handle them independently. I would like to have them all in a List / Array (In my example it loads 4 rows of 10 orbs each with each row with different color and different value for the text (from 1 to 10)).

    Right now, it works, I mean, it does what I want but before going further I would like to know if this is the way of doing it and if I'm understanding correctly.

    Please, feel free to make comments and I think it will be relevant for others, like me, that are learning :) But if you find it not relevant for others, please, just delete the post.

    Here is the code:

    Code (CSharp):
    1. using System.Collections;
    2. using System.Collections.Generic;
    3. using UnityEngine;
    4. using DG.Tweening;
    5. using UnityEngine.Rendering;
    6.  
    7. public class Game : MonoBehaviour {
    8.  
    9.     public class Orb
    10.     {
    11.         public int value;
    12.         public GameObject orbMesh;
    13.         public GameObject valueText;
    14.  
    15.         public Orb(int Value, GameObject OrbMesh, GameObject ValueText)
    16.         {
    17.             value = Value;
    18.             orbMesh = OrbMesh;
    19.             valueText = ValueText;
    20.         }
    21.     }
    22.  
    23.     public GameObject theOrbMesh;
    24.     public GameObject theValueText;
    25.  
    26.     List<Orb> completeDeck = new List<Orb>();
    27.  
    28.     void Start ()
    29.     {
    30.         Debug.Log ("Start");
    31.  
    32.         createAllOrbs ();
    33.         assignColorsAndNumbers ();
    34.  
    35.         Debug.Log("*** completeDeck ***");
    36.         listDeck (completeDeck);
    37.     }
    38.  
    39.     void createAllOrbs()
    40.     {
    41.         for (int z = 0; z < 4; z++)
    42.         {  
    43.             for (int x = 0; x < 10; x++)
    44.             {
    45.                 completeDeck.Add (new Orb(x+1, Instantiate(theOrbMesh, new Vector3 (-3.6f + (x * 0.8f), 0.67f, z * 0.8f), Quaternion.identity), Instantiate(theValueText, new Vector3 (-3.6f + (x * 0.8f), 1.35f, z * 0.8f), Quaternion.identity)));
    46.             }
    47.         }
    48.     }
    49.  
    50.     void assignColorsAndNumbers()
    51.     {
    52.         for (int i = 0; i < completeDeck.Count; i++)
    53.         {
    54.             if (i < 10)
    55.             {
    56.                 completeDeck [i].valueText.GetComponentInChildren<TextMesh> ().text = (i+1).ToString ();
    57.  
    58.                 MeshRenderer t = completeDeck [i].orbMesh.GetComponent<MeshRenderer> ();
    59.                 t.material.color = new Color32 (255, 146, 146, 255);
    60.             }
    61.             if (i >= 10 && i < 20)
    62.             {
    63.                 completeDeck [i].valueText.GetComponentInChildren<TextMesh> ().text = ((i+1)-10).ToString ();
    64.  
    65.                 MeshRenderer t = completeDeck [i].orbMesh.GetComponent<MeshRenderer> ();
    66.                 t.material.color = new Color32 (146,179,255,255);
    67.             }
    68.             if (i >= 20 && i < 30)
    69.             {
    70.                 completeDeck [i].valueText.GetComponentInChildren<TextMesh> ().text = ((i+1)-20).ToString ();
    71.  
    72.                 MeshRenderer t = completeDeck [i].orbMesh.GetComponent<MeshRenderer> ();
    73.                 t.material.color = new Color32 (159,255,156,255);
    74.             }
    75.             if (i >= 30)
    76.             {
    77.                 completeDeck [i].valueText.GetComponentInChildren<TextMesh> ().text = ((i+1)-30).ToString ();
    78.  
    79.                 MeshRenderer t = completeDeck [i].orbMesh.GetComponent<MeshRenderer> ();
    80.                 t.material.color = new Color32 (255,241,77,255);
    81.             }
    82.  
    83.         }
    84.  
    85.     }
    86.  
    87.     void listDeck(List<Orb> Deck)
    88.     {
    89.         for (int i = 0; i < Deck.Count; i++)
    90.         {
    91.             //to do things here :D
    92.             Debug.Log(Deck[i].value);
    93.  
    94.         }
    95.         Debug.Log ("Deck Items: " + Deck.Count);
    96.         Debug.Log ("******************************");
    97.     }
    98.  
    99.     void moveOrb(List<Orb> Deck, int index, Vector3 position, float Time, float Delay)
    100.     {
    101.         Deck [index].orbMesh.transform.DOMove (position, 3f).SetDelay (Delay);
    102.         Deck [index].valueText.transform.DOMove (new Vector3 (Deck [index].orbMesh.transform.position.x, position.y + 0.68f, Deck [index].orbMesh.transform.position.z), 3f).SetDelay (Delay);
    103.     }
    104.  
    105.  
    106.     // Update is called once per frame
    107.     void Update () {
    108.      
    109.     }
    110.  
    111.     // Update is called once per frame
    112.     void OnApplicationQuit () {
    113.         Debug.Log ("OnApplicationQuit");
    114.     }
    115. }
    116.  
     
    Last edited: Dec 22, 2016
  2. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    Looks reasonable to me.

    As a matter of style, the standard in these parts is to capitalize class and method names, but to not capitalize fields or local variables (including parameter names). So for example,

    Code (CSharp):
    1. void listDeck(List<Orb> Deck)
    really should be

    Code (CSharp):
    1. void ListDeck(List<Orb> deck)
    But otherwise, it all looks fine to me.
     
    jjgarcianorway and Kiwasi like this.
  3. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Wow! Thanks!!!

    Honestly I was not expecting to do it well! I was expecting something like this is not the way to do that xd

    But that encourage me to keep improving :)

    Can I continue my little project and share the code here with all you? Let's call it open source :)

    I think we can enrich this with all your knowledge so others can take profit of it :)

    Sounds like a good plan for you?
     
    JoeStrout likes this.
  4. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    If you want to do that, I highly recommend you set it up as a GitHub repo. Then submit the project to unitylist.com, which is a fantastic resource for finding other open source Unity projects.
     
    jjgarcianorway and Ryiah like this.
  5. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Ohhh thanks! I think this is very interesting! I will check how I can do it and continue updating here my progress :)

    I hope others find it interesting and I can learn with all your advices!

    Much appreciated!
     
  6. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Hello all,

    Sorry for the delay but I just upload the source code of my project: https://github.com/jjgarcianorway/Orben

    Remember that this is just a testing place where I'm trying to build a game :D

    Please, feel free to make any comments and suggestions and if you even want... Be in touch with me :)
     
  7. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Might not be a bad idea to have a proper Readme for the project if you really want people to use it and contribute to it. I for one am not really clear on what the purpose of your project is, as in, what the use case is and how it might benefit my projects. Detailing that in the Readme is a great way to get people involved.

    Also... I see that you've included a couple of assets in your project that appear to be from the Asset Store, several of which are paid assets. Including anyone else's work without permission is a huge no-no, and in the case of paid content, is most likely in direct violation of your usage license for that asset. You'll need to remove those from the repo, clean the repo out, and re-commit. Ensure your project does not rely on those assets to run, or you won't find anyone willing/able to collaborate with you.
     
    jjgarcianorway likes this.
  8. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Oops! My fault with the assets! I will remove it as soon as possible and sorry for that!

    Unfortunately, the project needs at least one of them for working... because it does the movements...

    I'm not in front of my computer but I will try to cleanup the repository right now. Again, sorry about that.

    Tomorrow I will check the README and I will try to explain it all better :)

    Again... thanks a lot for your good advice!!!

    Edit: the repository has been deleted right now and it will may be restored soon . Sorry!!
     
    Last edited: Jan 9, 2017
  9. hippocoder

    hippocoder

    Digital Ape

    Joined:
    Apr 11, 2010
    Posts:
    29,723
    Completely disagree but that's just style :) all my members start lowercase so...

    And agree with the fact the code presented by the op is perfectly fine. I just don't think you should be going about telling people to prefix things with uppercase.

    https://msdn.microsoft.com/en-gb/library/x2dbyw72(v=vs.71).aspx

    Each to his own :)
     
  10. JoeStrout

    JoeStrout

    Joined:
    Jan 14, 2011
    Posts:
    9,859
    Well, it's the system used by the Unity framework, as well as every bit of sample code from Unity that I've ever seen, and (therefore) by most Unity bloggers and tutorial writers. So it's worth at least being aware of it, so if you choose to be contrary, at least you're doing so consciously instead of accidentally. ;)

    (And I will say, when I download a code asset that doesn't follow this Unity standard, it annoys the heck out of me... one star off just for that!)
     
    Ryiah likes this.
  11. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Hello again!

    Just a question... If the code I would like to provide includes references to 3rd party assets (paid ones)... how should I do it? Should I just upload the script itself and inform about it needs that asset?

    Apologize for the newbie questions!

    Edit: I deleted part of the assets folder with the Paid Assets I had there. I think now is ok but I will appreciate if someone can check it :)
     
    Last edited: Jan 10, 2017
  12. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Honestly, for it to work as an open repository kind of thing, it just can't have paid assets in it. Or any third party assets at all, for that matter. But I bet when you really think about it, you don't need those assets.

    I remember seeing three asset packages. One was a visual effects pack, so obviously that's not needed. Another was ProBuilder, which I believe is for building prototype level geometry. Not needed. You can use primitive shapes just fine for a code base project like this.

    I think the other package was some kind of tweening package. Again, not necessary. Probably makes animation a bit easier, sure, but you're just moving simple shapes around. This can easily be handled with Lerping functions that work in vanilla Unity.

    Again, I'm not positive what the point of your code is, and what it's used for. But if the idea is that it's a tool that will allow you to do something, you shouldn't be forcing unnecessary dependencies on people who want to use it. It can be nice to build in compatibility with things, like if you want your asset to interface with PlayMaker nicely, for example, but you should never force it.
     
    jjgarcianorway and JoeStrout like this.
  13. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Ohhh I see. Thanks for your honest reply!

    I will check to replace the orb with a basic primitive in Unity and to change all movements and fading from DOTween to Leap.

    It will take me a while but I will do it :) (in the meanwhile I removed those folders from Assets)

    Your suggestions are orders to me :)

    Thanks a lot!!!
     
    Schneider21 likes this.
  14. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Not a problem. Once you have it pushed back up, I'll take a look at it and see if I can offer any further input. Even if it's a small, nitpicky change, I'll do some kind of pull request so you can get some experience merging code from the community.

    Good luck!
     
    jjgarcianorway likes this.
  15. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Seriously... You're the best! Thanks really! I will change the code, hopefully tomorrow... I need to check how Lerp works XD Replacing the Orb it's easy :)

    Super thanks!
     
  16. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    I do not know if I'm doing something wrong or not but it will take me a while to completely remove the DOTween asset.

    Now I learned how to do the Lerp but:

    a) The movement is quite smooth but it seems like I will need to implement some kind of Easing (I used OutQuint before)
    b) Seems like I will need to create my own FadeIn, FadeOut, ChangeColor,...
    c) All this needs to be with Delay option and that seems to be only possible through IEnumerator. Tested with my new MoveObject(GameObject objectToMove, Vector3 source, Vector3 target, float overTime, float Delay);

    Seriously there is nothing already implemented natively? At least I cannot find them!

    I will keep investigating :)
     
  17. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Here I am with the function I've created to change alpha...

    But I have a few questions... newbie questions XD

    Can you tell me how I can do it to use the same function but for Text instead of Image? What happens if the Image or Text is as a children?

    Should I do it directly for color instead of only alpha? (I think so).

    Many thanks for reading and helping me!

    Code (CSharp):
    1. IEnumerator ChangeAlpha(GameObject objectToChange, float source, float target, float overTime, float delay)
    2.     {
    3.         var i = 0.0f;
    4.         var rate = 1.0f / overTime;
    5.  
    6.         Color32 temp = objectToChange.GetComponent<Image> ().color;
    7.  
    8.         while (i < 1.0f)
    9.         {
    10.             Debug.Log ("temp: " + temp);
    11.             i += Time.smoothDeltaTime * rate;
    12.             objectToChange.GetComponent<Image> ().color = new Color32 ((byte)temp.r, (byte)temp.g, (byte)temp.b, (byte)Mathf.Lerp(source, target, i)); //(Time.time - startTime)/overTime)
    13.  
    14.             yield return delay;
    15.         }
    16.         objectToChange.GetComponent<Image> ().color = new Color32 ((byte)temp.r, (byte)temp.g, (byte)temp.b, (byte)target);
    17.     }
     
  18. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Alright, let's back up a second and cover the first things first:
    1. What does your project do? What is the overarching goal of doing this work?
    2. How will it be used? Is the idea that it would be imported as an asset package and used by others in their existing projects? Or is this going to be a game in itself?
     
    jjgarcianorway likes this.
  19. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Hello sir :)

    The project itself is just a simple game for practicing and learning (since I'm coming from Corona). The source code will be available so everybody is able to learn from it / improve it.
     
    Last edited: Jan 11, 2017
  20. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Okay. But what is the aim functionally of what you're trying to achieve here? I'll give you an example:
    My point is that it's much easier to tell you what direction you should move in if I have any idea what the end goal is. I can talk about Lerp functions and changing transparency all day, but if it's not relevant to what you want to do, it might not even be helping you.
     
    jjgarcianorway likes this.
  21. nreeves

    nreeves

    Joined:
    Jan 11, 2017
    Posts:
    2
    Looks pretty good to be honest, maybe things should be a little more spaced out but other than, it looks pretty good.
     
  22. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Ohhh sorry! I think understand now :)

    Before I was using DOTween for the movements and now I'm trying to replace it with Unity native tools.

    In the README I wrote (quick and dirty) what the project is doing and how it works. But summarizing, what I would like to have is:

    FadeIn: text, images, gameObjects (some with children)
    FadeOut: text, images, gameObjects (some with children)
    ChangeColors: text, images, gameObjects (some with children)
    MoveGameObject: text, images, gameObjects (some with children)

    All this, if possible with Ease and Delay.

    3 messages ago I placed the function I've created (for sure not perfect) and then some question were hammering my head and I ask them there :D

    But again... I'm newbie here and even I planned this game before (I made it in Corona) I think it can be a good exercise for learning here as it may have almost all the simple things we all need to start :)
     
  23. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Ah, gotcha. I hadn't checked your repo again since you re-uploaded it. You wrote quite a bit! I have to do a bit of work, but I'll look into it when I can and check back in with you later!
     
    jjgarcianorway likes this.
  24. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Ohhh don't worry! You must be busy!

    When you have time is ok for me :) And again... thanks for your help and contribution :)
     
  25. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Hello everyone!

    In the meanwhile, I'm still trying but... more questions :D

    What I'm trying?

    I want a function to Change Alpha in Time valid for different component types (UI, Images, Text, etc..).

    I call it using:

    Code (CSharp):
    1. StartCoroutine(gameTools.ChangeImageAlpha(consoleButton.GetComponent<Image> (), "Image", 0.0f, 255.0f, 2f, Delay));
    and is defined as:

    Code (CSharp):
    1. public IEnumerator ChangeImageAlpha( Component objectToChange, string compType, float source, float target, float overTime, float delay)
    2.     {
    3.         yield return new WaitForSeconds(delay);
    4.  
    5.         var i = 0.0f;
    6.         var rate = 1.0f / overTime;
    7.  
    8.         System.Type type = objectToChange.GetType();
    9.  
    10.         Debug.Log (type);
    11.  
    12.  
    13.         var component = objectToChange.GetComponent <type>(); //THIS DOES NOT WORK... BUT IT WOULD BE SO COOL
    14.  
    15.         //var component = objectToChange.GetComponent <UnityEngine.UI.Image>(); // THIS WORKS LIKE A CHARM
    16.  
    17.         Color32 componentColor = component.color;
    18.  
    19.         while (i < 1.0f)
    20.         {
    21.             i += Time.smoothDeltaTime * rate;
    22.             component.color = new Color32 ((byte)componentColor.r, (byte)componentColor.g, (byte)componentColor.b, (byte)Mathf.Lerp(source, target, i)); //(Time.time - startTime)/overTime)
    23.             yield return delay;
    24.  
    25.  
    26.         }
    27.         component.color = new Color32 ((byte)componentColor.r, (byte)componentColor.g, (byte)componentColor.b, (byte)target);
    28.     }
    The problem?

    Well this one is not working... Of course :)

    In the code I commented the line that fixes the error but... that does not solve my intentions :)

    As I would like to use it for other types like Text, for example... Is that possible?

    Thanks a lot!!!
     
    Last edited: Jan 14, 2017
  26. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Another great source you'll want to become familiar with, if you're not already, is the documentation. The listing for GetComponent shows that it has another form of the method that doesn't have a type set, but rather gets a string passed as a parameter indicating the type. So if you do:
    Code (CSharp):
    1. string objType = objectToChange.GetType().ToString();
    2. var component = objectToChange.GetComponent(objType);
    You shouldn't get a compile error. Now, whether this will work or not is an entirely different story. You'll have to try it and see.

    One problem I see rising from this is you'll need to do a lot of type checking and branch your logic accordingly. For example, you're accessing component.color and modifying it in your loop. Depending on what type of component you're actually accessing, there might not be a color property. You'll have to see what kind of component you're working with using GetType again, and handle different cases appropriately.
     
    jjgarcianorway likes this.
  27. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    Hello!

    Thanks for your reply!

    Unfortunately I tested that before, that is the reason of having an string in my function and it does not give any compile error there... but it t gives one on the line below when it tries to use the component.color but is not able to find it (I guess because is not smart enough to know what it will have).

    Edit: I updated the code in Github and is commented but as you will see, the next instruction gives error as it does not "understand" what type contains...

    Maybe I should just have a function per each type?
     
    Last edited: Jan 18, 2017
  28. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    I think I'm overcomplicating things for myself removing the DOTween asset... Maybe I'm giving up too quick but... Oh! It is really well done!

    I'm not trying to sell it to anyone but... how the heck is not included something like that by default in Unity? I think is too basic stuff!

    I'm seriously trying to have something that allows me to do easy alpha changes, color changes, etc... with ease,... but is like reinventing the wheel when the solution is there and is so cheap!

    I really want to continue sharing the code with all you but I can understand that code cannot be shared if it forces you to use a plugin... and even more if it costs money.

    So... what is the solution? I do not know.

    Thanks for reading!
     
  29. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    What the heck... I'm sure I posted a response to your previous post, but it's not here! I'll try to recap, along with an updated response based on your last post.

    Now with all that said, if you really just want to make forward progress, using your purchased asset is the way to go. You won't be able to share it with the community, but that's fine. Your focus at this stage should be growing yourself anyway. There's no wrong way to do it, as long as you're learning and moving forward.
     
  30. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    hehehe don't worry sir :) I like the way you think.

    Honestly I would like to share it with the community but I also think that is is causing me to much time and effort for something that is already implemented.

    Maybe I should just create a new thread and explain my progress there and share with the community but in a completely different level. Maybe like you do in your "Uncharted Galaxy DevLog" :)

    Do not know really but your advise is always welcome :)

    My wish to have a partner that likes to explore and learn together but it's difficult of course, everybody has their own projects!

    Again and as always, thanks!
     
  31. Schneider21

    Schneider21

    Joined:
    Feb 6, 2014
    Posts:
    3,512
    Absolutely, man.

    Definitely consider throwing together a thread in the Works In Progress. Just make sure it's in the correct format for the subforum. And include a lot of pictures, because people just love pictures.

    But I would say for now, the best thing you could do would be to complete what you want to accomplish using all the available tools you can. And when you finish, if you still feel strongly about sharing with the community, look at removing third party assets at that point to get the project in a position where it can be shared.

    Keep at it! Good luck!

    PS - Check out the collaborate forum for finding someone to work with! It takes a while to get a response, sometimes, but it's there!
     
  32. jjgarcianorway

    jjgarcianorway

    Joined:
    Oct 19, 2015
    Posts:
    33
    I think I will follow your advice...

    Honestly I would like to appreciate the time and effort you made for helping me and I hope it will be useful for others too :)

    Right now I will focus on completing the "game" or at least do something visible and then, if everything is ok, I will share it with the community removing the 3rd party assets.

    I will check also the collaborate forum just in case that someone wants to do something together :)

    Again... thanks a lot. Not enough words to appreciate what you did :) I hope we continue in touch from time to time :)

    Good luck sir!
     
    Schneider21 likes this.