Roblox / Core-Scripts

All of ROBLOX's core client scripts.
Apache License 2.0
253 stars 184 forks source link

OOP design on the RootCamera breaks several standards. #158

Closed Quenty closed 4 years ago

Quenty commented 9 years ago

The root camera is trying to do several functions at once, when in reality behavior can be broken into multiple reusable components. This makes modifying the RootCamera (and subsequent versions of cameras) very difficult.

Behaviors the RootCamera handles (which should be separated by Encapsulation)

Each of these behaviors are interesting, but should not be linked as a core component of the RootCamera.

Base behavior

Rather, the RootCamera should only have the following APIs

function RootCamera:GetCameraLook()
function RootCamera:GetCameraZoom()
function RootCamera:GetCameraActualZoom()
function RootCamera:ViewSizeX() -- Note this method and the one below should start using the camera.ViewportSize
function RootCamera:ViewSizeY()
function RootCamera.new()
function RootCamera:RotateCamera(startLook, xyRotateVector)
function RootCamera:ScreenTranslationToAngle(translationVector)

New API

function RootCamera:SetCamera()
function RootCamera:AddChild(ChildModifier)
function RootCamera:RemoveChild(ChildModifier)
function RootCamera:Destroy() -- Removes events, destroys children

Children/API suggestion

Currently, the RootCamera has one behavior, but we know this is going to change as ROBLOX goes on.

RootCameras should be extended via children, or some sort of plug-and-chug input or behavior handling items. For example, the current player's character is tracked, and behavior happens when it joins. A class HumanoidTrackingExtension would handle this behavior nicely. The RootCamera could then loop through these behavior classes and update them, et cetera. The following API could occur

function HumanoidTrackingExtension:IsInFirstPerson()
function HumanoidTrackingExtension:CalculateOffset()
function HumanoidTrackingExtension:HandleTransparency()
...
function HumanoidTrackingExtension:Destroy()

This sort of behavior should also be done with the input, so input can easily be swapped in mode, and input mode is easy to track and reusable in other camera extensions, instead of creating an inextensible RootCamera that is not extendable. Note that I've suggested that MouseLock and Mouse and Keyboard all be handled differently because these are inherently different input types that change input behavior --> camera behavior.

Another example child would be a InputSystemExtension which selects which input mode to use. At this point, the InputSystemExtension would be able to switch the strategy of input based upon the existing camera mode. These children would interact directly with the RootCamera API calling methods, with the RootCamera knowing of them. This means the RootCamera has \ few to no dependencies**

The following variables should also be camera variables

workspace.CurrentCamera --> self.Camera

Allowing the camera and other small variables like this to be set means that

Effects of such a change

Although this is a very large refactoring, this also does a lot of things for you

Quenty commented 9 years ago

I should note that I highly suspect there will be no change, but I find it worth it to submit a request for change anyway.

einsteinK commented 9 years ago
function RootCamera.new()
function RootCamera:SetCamera()

With those 2 APIs, it would be able to create cameras, and set which one "renders", right? That would be a very nice feature, very useful, also for your case, for special cameras. (It's easier to create 3 separate cameras for special things, than making one complicated camera)

Quenty commented 9 years ago

Exactly! I wanted to be able to blend cameras together to warp between modes too.

einsteinK commented 9 years ago

Blending would be a completely different thing, probably specific for every game. It would be better if the game creator just creates a new camera for the blending. (or utilises one of the two cameras he's blending)

jmargh commented 9 years ago

This post is really awesome and we are open to changing these for sure. However, right now our priority is to ship this feature and get it in a stable state. Refactoring the code at this time would add complexity to project.

We will keep this issue open and revisit it at a later time when we feel the new camera and controls are stable, and can start making incremental changes to making the system better.

ghost commented 7 years ago

This issue was moved to GitBlox/Core-Scripts#6

Validark commented 7 years ago

Who is going to write this?

cliffchapmanrbx commented 4 years ago

Hi there, thank you for your feedback. We're going to be closing this repository as we've chosed the Roblox Dev Forum as our venue for developer feedback. Please feel free to migrate your feedback to a DevForum post if you feel it is still relevant.