Search Unity

  1. Unity 6 Preview is now available. To find out what's new, have a look at our Unity 6 Preview blog post.
    Dismiss Notice
  2. Unity is excited to announce that we will be collaborating with TheXPlace for a summer game jam from June 13 - June 19. Learn more.
    Dismiss Notice
  3. Dismiss Notice

Official Allow multiple entry/exit points in Locations

Discussion in 'Open Projects' started by georgefnix, Dec 9, 2020.

  1. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    Amel-Unity and cirocontinisio like this.
  2. DSivtsov

    DSivtsov

    Joined:
    Feb 20, 2019
    Posts:
    152
    Will be good to discuss the your ideas before you begin, based on good traditions.

    P.S>
    I think good style to add to your post the blue mark Official, it can be set after creation thread also.
     
    georgefnix likes this.
  3. shuttle127

    shuttle127

    Joined:
    Oct 1, 2020
    Posts:
    183
    Creating this post does not "assign" a task to you, as community work isn't officially "assigned" to one person. Anyone is allowed to work on a community card to give as many people the opportunity to contribute as possible, so feel free to fork the repository and create your own PR. Excited to see how you tackle this. :)

    While I can agree that explaining implementation is a good idea, what I've seen lately is that forum threads for cards mainly discuss implementation for complex problems and not straightforward first tasks like this.

    Like I said in the other thread, @cirocontinisio is the one that usually gives the "Official" mark, and for this particular task, it's not really necessary because it's a straightforward "good first task" that shouldn't involve too much implementation discussion.

    For example, there was a good first task a little while ago that Ciro shared a video of how to implement in GitHub comments, and no forum thread was created. I waited a few days to see if the people in that GitHub discussion would get to it, and when they didn't, I took a crack at it and completed it without creating a new forum thread saying I would, just a PR when the changes were in. From that experience, it seems to me that "good first tasks" aren't meant for collaboration and really just a way to get people the positive feeling of having a contribution to the project to build confidence to help with something more complex in the future.
     
    georgefnix and Erethan like this.
  4. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    I do not know how to do the blue mark thing.

    It seems from the card they want the player to be moved to a specific spawn point in another scene when triggered.

    I was thinking about having a scriptable object represent the spawn points(name, scene name, location, rotation). A simple script would handle the scene change and would take an object as reference. Other scripts(like a trigger volume, dialogue manager, etc) would give that script their point reference and ask it to execute the load. When the scene loads the character controller and camera move themselves to the needed location and rotation.

    This is my initial ideas on this, but I expect there to be minor changes as I start working on it(if cleared to do so).
     
  5. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    Based on shuttle's post I am going to start work on this.
     
  6. shuttle127

    shuttle127

    Joined:
    Oct 1, 2020
    Posts:
    183
    For sure, go right ahead and do an implementation. This is supposed to be a learning project, so what better way to learn than by doing? :)

    There have been many people that made their first post ever (like you) to ask to be assigned a task on this project, and without a response, those people haven't been heard from on the forums again. It is great to ask for permission, but on something like this, the worst that could come of someone creating a PR to address an issue "without permission" is that it doesn't get accepted, but at least that person would have the experience of doing the implementation.

    To tell another story, there was a discussion about making it easier for people to contribute on Discord a few weeks ago, and a lot of it was just not knowing that it was okay to try something. The other big thing was just the sheer overwhelming complexity of the project itself, which is why Ciro has been really great at trying to classify tasks as green/yellow/red for complexity. The only way someone new is going to ramp up is to start small, get some easy wins to build confidence, and then who knows, maybe one day soon contribute a bigger feature.
     
  7. DSivtsov

    DSivtsov

    Joined:
    Feb 20, 2019
    Posts:
    152
    If you are the author of thread, you can edit the thread. In the upper part (in the Edit mode) of your thread will be link Thread Tools, from it you can select the Edit Title of this thread and set thread prefix.

    You can get started :) nothing can stop you, the @shuttle127 (and I :) ) is very serious :):cool:
    But you must know - the main aim of this project it's a learning the good style Unity programming. Which achieved by discussion different ideas and selection the best (optimal) solution. And if you want, that your realization will be finale realization which included implement all good ideas. Will be good to discuss it, before you spent to it many time, because exist possibility, that somebody give more optimal solution and the Community decide that it more appripriate for this project.
    But if you ready to produce "some prototype" which after it will discusse by Community , not spending many your time. it's also ok.
     
    Last edited: Dec 10, 2020
  8. DSivtsov

    DSivtsov

    Joined:
    Feb 20, 2019
    Posts:
    152
    I hope you see Game Architecture Overview and Event system (from Wiki) which is the base for Scene Loading in this project.
     
    georgefnix likes this.
  9. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    I noticed those things after looking at the code(after I made that post), and I intended to tailor my solution to fit with what already existed. Still thank you very much for showing me to the wiki. That is some very useful information.
     
  10. cirocontinisio

    cirocontinisio

    Joined:
    Jun 20, 2016
    Posts:
    884
    Hey @georgefnix, welcome to the project!

    I don't think regular users can mark threads as Official, only admins should be able to. I'll set it as official.
     
    georgefnix likes this.
  11. DSivtsov

    DSivtsov

    Joined:
    Feb 20, 2019
    Posts:
    152
    I checked it, It's possible now, @cirocontinisio May be good idea to block it and make corresponding note regarding creation of threads for cards from road map
     
    Last edited: Dec 11, 2020
  12. Amel-Unity

    Amel-Unity

    Unity Technologies

    Joined:
    Jan 30, 2020
    Posts:
    62
    This is great @georgefnix ! Yes it's a good idea to have a scriptable object to represent a spawn point. We can pass it as a third parameter to the scene loading event we are currently raising when we exit a location. This object would represent the entry point of the location to load. We will then have 2 MonoBehaviour scripts, the exit point one using a trigger (the one we already have) and the entry point one which writes to this scriptable object.
     
    georgefnix and canchen_unity like this.
  13. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    Thanks @cirocontinisio . Sorry for the delay, yesterday was trying to understand the systems and scripts that are already in place and working on a solution that fits within the current systems.

    At the moment, but this may change:
    The Scriptable Object representing the location(name TDB) is used in a new event the LocationLoader is listening to(I am not going to remove anything that already exists). The location loader does its normal thing, but once all of the scenes are loaded it raises a new event that communicates where to move the protagonist. Either the spawner or the protagonist(TBD) are listening and move to that new location. The exit code will have to be changed to have a parameter that accepts a SO representing location.

    Edit: Amel, I read your post. That is very interesting and I will give it thought. I didn't consider the use of scriptable objects in that way.
     
  14. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    Zfighting.PNG

    Currently the loader accepts multiple scenes from an array to be loaded additively. It sets the first element of this array to be the active scene. Despite there being an active scene, it appears the other scenes are still being rendered. In the attached image(which is supposed to be beach) you can see forest's trees as well as significant z fighting of the ground textures.

    I am uncertain how this should be addressed. Either we do not load multiple scenes, or we disable the non-active scene elements(or some similar solution I am yet unaware of).

    The other object I am contemplating. I am considering using the locationSO for my task's purpose. To wit, I would need to add two Vector3's to that object. This obviously changes what the SO was initially defined for(to delineate between scenes for menus and scenes for gameplay), but it is exactly descriptive of what the task requires. I was initially going to make another SO with a similar name(ex: localeSO or placeSO), but a designer could easily get confused which SO they need to create. I seek feedback if this matters.
     
  15. Amel-Unity

    Amel-Unity

    Unity Technologies

    Joined:
    Jan 30, 2020
    Posts:
    62
    We wanted to allow to load multiple scenes at first because we thought we could have some locations split in multiple scenes. Now that we have a clearer idea of how the game will be like, we will have a scene for every location. So maybe we can remove the ability to load multiple scenes as it is apparently a bit confusing (and especially if we will not use/need it).

    It is better if we keep the locationSO and the entrypointSO separated especially considering that one location can have multiple entry points.
     
    georgefnix likes this.
  16. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    Thank you Amel. I made quite a few changes based upon your input especially.

    Regardless, this is my PR. It also covers card 197 since that uses the changes to card 195(and it was a small addition to my other work).

    Edit: Based on feedback from my original PR I made an updated PR.
    It can be found here:
    https://github.com/UnityTechnologies/open-project-1/pull/269

    I also made a video demonstrating my PR. It may not be done processing by the time you see this.

     
    Last edited: Dec 16, 2020
    itsLevi0sa likes this.
  17. DSivtsov

    DSivtsov

    Joined:
    Feb 20, 2019
    Posts:
    152
    Quick reply after first look:
    You use term the Location for points of spawn on scene? It's not good, because currently "the Location in Editor" it's a some special type of SO.
    IMHO it is more appropriate to change his name to something else - Scene point / spawn point and that kind of thing
     
  18. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    It isn't just points of spawn on scene. It is points in a scene that can be used for anything including spawning a character(or teleporting, or whatever). For example, if an npc wants to walk to the clocktower they could query that location and begin pathing. In my opinion, in the context of video games a scene could represent a location, but in many cases it would represent an area(that has locations, ex: a town that has a market, bank, bridge, etc).

    If it is important to the team I am not completely opposed to changing the names of the scripts.
    Currently:
    Scenes are represented by SceneSO
    Locations in those Scenes are represented by LocationSO

    If SceneSO is to be changed to LocationSO. Then LocationSO should be named something that isn't a synonym of location(like place). PointOfInterestSO or PointSO would be my suggestion.

    Update: I am in the process of changing them to my second suggestion(which happens to be one of DSivtsov's and is more general). It is a longer task then I originally envisioned as I have more to rename than the classes.

    Update 2: Changes made. All instances(I could find) of Scene is now Location, and every Location is Point.
     
    Last edited: Dec 17, 2020
  19. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    After reading the other thread(about card 197) and some thought, I have decided to remove my request to merge. The other card is closely related to this one(that is why a two additional scripts to 195 solved 197 for me), and because I changed aspects of how things were structed I feel like my solution steps on too many toes.

    I do not regret this at all. I learned a lot, especially that I need to improve my communication. Thank you all. I will probably be back at some point with an improved process.
     
    Last edited: Dec 17, 2020
    canchen_unity and cirocontinisio like this.
  20. georgefnix

    georgefnix

    Joined:
    Jan 24, 2014
    Posts:
    10
    I took yesterday off, and with that time I realized that I still understand this problem well and I can contribute in a way that doesn't mess with the way things are done. In this pull request I tried to change as little as possible.

    Pull Request: https://github.com/UnityTechnologies/open-project-1/pull/275

    PS: Thank you for the likes and responses to this thread.
     
    cirocontinisio likes this.
  21. cirocontinisio

    cirocontinisio

    Joined:
    Jun 20, 2016
    Posts:
    884
    Will take a look at it soon!
     
    georgefnix likes this.
  22. rcarag

    rcarag

    Joined:
    Feb 11, 2016
    Posts:
    5
    Edit: I have created a PR of my approach
    PR: https://github.com/UnityTechnologies/open-project-1/pull/284
    How to use: https://imgur.com/a/kEiDOZm

    ---

    Hi all, I've been studying @georgefnix's PR and thought it can be improved to avoid putting additional responsibility on the Protagonist class to teleport to its spawn position.

    Since the SpawnSystem already includes provisions for defining multiple Spawn Locations, perhaps we can just add enhancements to pick an appropriate Spawn Location, based on data passed by the LocationExit taken by the Protagonist.


    I'm still getting around the architecture, but I'm currently trying this solution:

    1. We create "paths" to link location entrances and exits using ScriptableObjects. Each path is just an SO asset with a sensible name (ex. "BetweenBeachAndForest", "BetweenBeachAndTestingGround").
    2. Each LocationExit is assigned a path (ex. the ExitToForest in the screenshot above would be assigned the "BetweenBeachAndForest" path). Once the LocationExit is triggered, it saves the PathTaken to a runtime anchor.
    3. Add new Spawn Locations to the SpawnSystem as above. Spawn Locations that represent entrances coming from a path will have a LocationEntrance with a path assigned to it.
    4. When the next location loads and SpawnSystem starts, it checks the PathTaken runtime anchor.
    - If a path is not set, spawn using the Default Spawn Index.
    - If a path was set, find and use the Spawn Location with a LocationEntrance whose path == PathTaken. If not found, fallback to Default Spawn Index.

    *EntranceFromTestingGround is unused because the current TestingGround scene doesn't actually have an exit leading to the Beach scene yet.

    The downside to this approach is the SpawnSystem will become very tightly coupled to this responsibility. But the good thing is that SpawnSystem can still fallback to its default spawn location ("Location 01") and continue to work as before. Also, each Location doesn't need to know anything else about other locations, other than the Path that links between them. And entrance locations can also be visualized while editing the scene, as above. Let me know what you think about this approach so far.

    Prior to this, I was exploring the following:
    - An FSM or some kind of game manager could be used to determine the location entrances, but it seemed overkill given the early state of the game and could be difficult to change for testing purposes.
    - PathTaken could be passed via the LoadLocation event channel, but it wouldn't make sense when loading menus.
     
    Last edited: Dec 29, 2020
    itsLevi0sa, canchen_unity and Revaliz like this.
  23. Peter77

    Peter77

    QA Jesus

    Joined:
    Jun 12, 2013
    Posts:
    6,640
    It's so much more valuable to have such video, that goes over the changes, than just a PR! So thank you for recording it :)
     
    itsLevi0sa and georgefnix like this.
  24. Amel-Unity

    Amel-Unity

    Unity Technologies

    Joined:
    Jan 30, 2020
    Posts:
    62
    Hey everyone :)

    @georgefnix Thanks a lot for the PR and for making the video. I looked into the latest PR you opened and to give some feedback: In the Protagonist class, I would avoid using the check in the update. You could use events for that instead. I also agree with @rcarag on avoiding to put extra responsibility on the Protagonist class and it seems like @rcarag got inspired from your PR and decided to also create one himself where he improves the solution so thanks again for working on it.

    After looking at the PR @rcarag opened, I like the approach you used and building on top of the SpawnSystem.
    I will merge your PR, I will just tweak one thing before merging: regarding the _spawnLocations array in the SpawnSystem, we currently need to fill it manually in the inspector and if we later decide to add new entrances, we might forget to include them so it's error-prone. To avoid this, one way I thought of, is to add a tag ''SpawnLocation'' and assign in for every entrance. That way we can fill the array using FindGameObjectsWithTag (and we are using it only once when we enter a location before we spawn the player).
    I will also make a prefab for entrance which comes with the entrance script and the tag assigned. That way we can easily drag and drop the prefab in the scene each time we want to create a new entrance and we only need to specify the path. ( Also because it's a prefab, if we make any changes on an entrance, we don't need to push the whole scene anymore)
     
    rcarag likes this.
  25. Ignurof

    Ignurof

    Joined:
    Jul 4, 2019
    Posts:
    4
    Does this now count as completed? Was looking at the card and wanted to work on a solution but it looks like it is solved now.
     
  26. cirocontinisio

    cirocontinisio

    Joined:
    Jun 20, 2016
    Posts:
    884
    It's definitely "in" in the sense that we have a working solution, but I don't think it's fully complete. And it can always be refined.
    So I wouldn't suggest to create another different solution, unless you see something broken with the current one that can't be fixed.
     
  27. Ignurof

    Ignurof

    Joined:
    Jul 4, 2019
    Posts:
    4
    Fair enough, I figured as much.
    Will take a look at the possibility of making the entry/exit location module even better.
     
    cirocontinisio likes this.
  28. rcarag

    rcarag

    Joined:
    Feb 11, 2016
    Posts:
    5
    Hi Amel, thanks for introducing the convenience features to the system. However, I noticed that it also broke the SpawnSystem's original ability to be usable as a default spawn location when no other spawn location exists. I've created an Issue and PR for this.

    Issue: https://github.com/UnityTechnologies/open-project-1/issues/333
    PR: https://github.com/UnityTechnologies/open-project-1/pull/334

    Hope this helps for the meantime.
     
    Smurjo likes this.
  29. Harsh-NJ

    Harsh-NJ

    Joined:
    May 1, 2020
    Posts:
    315
    I also identified it but thought it is OK. The part that is getting the Game objects with tag is returning in an order where the Location 01 spawn point is at the last of the returned list, top two are entrances from locations.
     
  30. rcarag

    rcarag

    Joined:
    Feb 11, 2016
    Posts:
    5
    In scenes that don't have other spawn locations, ex. TestingGround_Small, the player wouldn't spawn and it would throw this error: "Exception: No spawn locations set." The fix I pushed should allow SpawnSystem to work as the default spawn location again.

    I remember the game was being designed to allow any scene to be quickly tested by opening it and directly pressing play on the editor. The recent commit id# 6d2c22a kind of broke that, so I figured a fix is necessary.

    Also, the fix will guarantee that when a scene containing other spawn locations (ex. Beach) is played by directly pressing play on the editor, the player will spawn at the position of SpawnSystem instead of a random spawn location. I believe this behavior makes it clear that the player has entered the location without a PathTaken, or that none of the other entrances matched the PathTaken. This could be useful for spotting unintended entrance/exit path configuration errors in the future.
     
    Harsh-NJ and cirocontinisio like this.