Add method to easily mass-create regions - #1
Conversation
There was a problem hiding this comment.
The code of the changes is fine, but I am not sure about the concept. Innovation Inc Thermal Power Plant stores all regions as data with the bounding boxes made from the relevant models. This design requires you to manage your regions along the models, which may be good or bad. You also then need to manage another source of truth to manage which regions can see which.
A unit test for this behavior would also be requested.
That is a good way of handling setup. Basing loads on camera location along with zone intersections makes it easier to subdivide the regions. This could be converted to automatically add regions based on model bounding boxes instead of requiring you to build your own zones like this would add. Both could co-exist, but there's a cross-over of use cases. |
Re-elaborate on README
need to setup my TestEZ again to test this, so bear with me.
|
I am still not sure how to feel about dealing with 2 (or 3 if you include the models) sources of truth. You will have your region stored as parts and still need to store the connections somewhere else. The README doesn't provide any guidance on this. |
getting used to this code style, sorry about all the commits! don't have github desktop connected for this one rn
|
There's probably a better way to handle the For loop in a299d47, not completely used for strict types yet. Added a new test case (it does pass!), updated types, tried to re-elaborate on README (still may need to come back to it), updated RegionState & BufferedRegionState to include the method. Created a working use case in my own world as well. Please let me know if there is still anything to resolve. |
TheNexusAvenger
left a comment
TheNexusAvenger
left a comment
There was a problem hiding this comment.
From the previous comment, I am still not sure about this idea. While it does regions, you will still need to manage a second source of truth for managing the connections.
| RegionState:ConnectRegions("Region1", "Region2") | ||
| RegionState:ConnectRegions("Region1", "Region3") |
There was a problem hiding this comment.
(No change) This was a good find. I completely missed that when copying from the unit test.
| local RegionState = RegionCulling.RegionState | ||
|
|
||
| RegionState:InsertRegionsFromInstance(ReplicatedStorage:WaitForChild("Regions")) | ||
| --// RegionCulling will generate all regions based on the provided instances. |
There was a problem hiding this comment.
Not a fan of a future-tense comment after the action instead of a present-tense comment before. --// is also not used in any other code blocks.
| if not Inst:IsA("BasePart") then | ||
| continue | ||
| end | ||
| local RegionPart : BasePart = Inst :: BasePart |
There was a problem hiding this comment.
A) Is using : BasePart on the left side needed when :: BasePart is on the right side?
B) If the : BasePart is needed, the rest of the code does not have a leading space.
| easier customization. | ||
| Name each region with the appropriate title and then add parts to determine their region. | ||
| --]] | ||
| function RegionState:InsertRegionsFromInstance(Instances: Model | Folder | Instance): () |
There was a problem hiding this comment.
Typing in Types.lua disagrees with Model | Folder | Instance. Instance should cover this without any issue.
| easier customization. | ||
| Name each region with the appropriate title and then add parts to determine their region. | ||
| --]] | ||
| function BufferedRegionState:InsertRegionsFromInstance(Instances: Model | Folder | Instance): () |
There was a problem hiding this comment.
Typing in Types.lua disagrees with Model | Folder | Instance. Instance should cover this without any issue.
| RegionStateObject:InsertRegionsFromInstance(InstanceTree) | ||
| expect(RegionStateObject:IsInRegion("Region4", Vector3.new(0, 1, 0))).to.equal(false) | ||
| expect(RegionStateObject:IsInRegion("Region4", Vector3.new(0, 3, 0))).to.equal(true) | ||
| end) |
There was a problem hiding this comment.
Would it be reasonable to add a test case for an empty instance? expect(function() RegionStateObject:InsertRegionsFromInstance(Instance.new("Folder") end).to.throw("Instance \"Folder\" has no child instances, unable to build Regions from it.") should work.
2 participants
A good way of pushing devs towards a likely easier way to setup a large amount of zones. Updated the README to include this as well. Follows code type-checking and format.