Changes in fov resolution #3071

0shine0 · 2017-08-16T21:34:58Z

Everything works as before in gameplay. The changes here are required for introducing sniper in CombatSystem module.
Pinging @Cervator and @portokaliu for review and merge.

GooeyHub · 2017-08-16T21:42:30Z

Hooray Jenkins reported success with all tests good!

emanuele3d · 2017-08-16T22:16:50Z

engine/src/main/java/org/terasology/logic/players/LocalPlayerSystem.java

@@ -97,7 +97,7 @@
    private LocalPlayer localPlayer;
    @In
    private WorldProvider worldProvider;
-    private Camera playerCamera;
+    public Camera playerCamera;


Why this change?

From the PR alone it seems unnecessary. If it's needed elsewhere it's good practice to comment your own PR about changes that might be puzzling. 😉

The camera present here is needed in another system. Thats why made is public, though a getter method would have been better. But the camera is to be refactored in coming months so i just did it public.

How about you get it from the WorldRenderer then? It's the object that instantiates the camera in the first place and you can find it on the Context/CoreRegistry. No point in going through intermediaries if it can be avoided.

Yeah, that would be better. But @In annotation doesn't work on WorldRendererImpl. I think i will need to get it directly from context. Will make the changes today :)

Getting it from the Context is even better. In theory, having spoken with Immortius some time ago, we'd like to move away from annotation-based injection and use the context (and sub-contexts) more widely.

That been said:

@In private WorldRenderer worldRenderer;

doesn't work? It should.

emanuele3d · 2017-08-16T22:18:43Z

engine/src/main/java/org/terasology/logic/players/LocalPlayerSystem.java

-        } else {
-            playerCamera.resetFov();
+        if(charMovementComp.mode != lastMode){
+            if (charMovementComp.mode == MovementMode.GHOSTING) {


And by the way, what's the GHOSTING mode?

IIRC it is literally the ghost movement mode you can enter via the game console. It changes the camera a bit.

Yeah, cervator is right. Previously it used to change the camera as per the mode in every loop. Now it changes the camera only when the mode is changed.

Ah, thank you. I can see where the "ghost" term comes from - going through walls.

I wish we could find a term that was a bit more explicit in its meaning. But perhaps just a short comment beside the constant in MovementMode would help. I.e.

GHOSTING(0f, 4f, false, false, false, 5f, true), // enables flying and going through solid surfaces

I checked and it seems the only mode that is somewhat ambiguous.

noclip is the traditional meaning for a 'ghost' (travel through things) mode in engines, if you are looking for a more specific name. Where flying is the traditional term for being able to fly unbounded. And both combined is usually not given its own name though some things of recent use spectator mode for that, though that one is often non-interactable.

noclip sounds like a very technical term that only people familiar with the opengl meaning of clipping might understand.

It's been used in cheat codes since back to Doom days idnoclip. ^.^

Ahh good ole Doom days :-)

But yeah noclip may make sense if you're a seasoned gamer, but just the term on its own always stuck out to me as weird.

We have a separate flight command that works without the noclip or camera tweaks. Could ghost be something else? Prolly. spectator I would think of as something else (having no body)

Ok, if we are brainstorming about this:

immaterial mode, intangible mode, wisp mode, incorporeal mode... oh... I like incorporeal.. let me find some synonyms... bodyless, disembodied, discarnate (yuck!), spectral, spiritlike, ethereal, transcendent, supernatural, nonphysical...

I guess incorporeal is the one I like the best, because I feel most people instinctively would understand that something incorporeal can go through walls. That been said, incorporeal might twist the tongue of some teenager and spiritlike is a more dumbed down contender. Except that some spiritlike modes, i.e. in World of Warcraft, still abide to -some- of the laws of physics, i.e. they must follow the landscape rather than fly through it.

So. Still vote for incorporeal mode. It seems very clear, unambiguos to me. But sticking to ghost (plus comment) or switching to spectator (or maybe observer? watcher? naah) would be fine too.

I was also thinking that terms such as spectator, observer and watcher might have their use in specific game modes.

I was also thinking that terms such as spectator, observer and watcher might have their use in specific game modes.

Yep, agreed.

incorporeal is a cool term, even if a bit long. wisp is short and kinda neat, but pretty close to ghost and not obvious. Maybe we should drop in a TODO or issue to consider the rename as part of the movement refactor since that could involve tweaking at the movement mode. Maybe of interest to @portokaliu

emanuele3d

Just a couple of small requests.

emanuele3d · 2017-08-17T12:26:58Z

engine/src/main/java/org/terasology/logic/players/LocalPlayerSystem.java

@@ -97,7 +97,7 @@
    private LocalPlayer localPlayer;
    @In
    private WorldProvider worldProvider;
-    private Camera playerCamera;
+    public Camera playerCamera;


How about you get it from the WorldRenderer then? It's the object that instantiates the camera in the first place and you can find it on the Context/CoreRegistry. No point in going through intermediaries if it can be avoided.

emanuele3d · 2017-08-17T12:39:14Z

engine/src/main/java/org/terasology/logic/players/LocalPlayerSystem.java

-        } else {
-            playerCamera.resetFov();
+        if(charMovementComp.mode != lastMode){
+            if (charMovementComp.mode == MovementMode.GHOSTING) {


Ah, thank you. I can see where the "ghost" term comes from - going through walls.

I wish we could find a term that was a bit more explicit in its meaning. But perhaps just a short comment beside the constant in MovementMode would help. I.e.

GHOSTING(0f, 4f, false, false, false, 5f, true), // enables flying and going through solid surfaces

I checked and it seems the only mode that is somewhat ambiguous.

Cervator · 2017-09-06T22:43:12Z

Bump. I believe the associated module PR Terasology/CombatSystem#32 got merged (maybe inadvertently from a second thing in the same branch) - does it actually work then?

Pinging @0shine0 for an update sometime and to also respond to comments here at your convenience so we can move forward :-)

0shine0 · 2017-09-08T17:04:10Z

@Cervator the combat system pr was merged but the sniper won't work unless this pr is merged in engine too. I have successfully tested the changes and they do not really affect anything but just provide me with functionality to implement sniper scope.

oniatus · 2017-09-08T17:16:24Z

Have you tried the other way to access the camera which emanuele3d proposed?

0shine0 · 2017-09-08T17:42:14Z

I had tried it, it gave an exception. So i had reverted back to getting camera from LocalPlayerSystem. I think the exception was due to the camera being returned null.

oniatus · 2017-09-08T17:54:53Z

Can you provide me a link to the calling code/PR which you need?
I would like to look for a better solution in this case because breaking encapsulation looks cheap but it adds even more cost later on.

A bit deeper explanation: At the moment, the LocalPlayerSystem is (ranting, sry 😄 ) a grown mess where the main responsibility looks like handling player input logic, aka mapping input events with some rendering stuff and item usage thrown over. If we now break up the field encapsulation, the class will also become an implicit camera provider for the combat logic and a refactoring of this thing will be more painful than it is now. And we don't want to make things worse, we make them better 😎

0shine0 · 2017-09-08T17:59:24Z

I totally agree. I will also look for a better solution.
Btw here is the code which uses camera if you wanna have a look.
https://github.com/Terasology/CombatSystem/blob/547924ec4ebe231422918149ac543783e3999373/src/main/java/org/terasology/combatSystem/weaponFeatures/systems/SniperScopeSystem.java

oniatus · 2017-09-08T18:22:45Z

Went for a short digging session. The class responsible for creating the camera is WorldRendererImpl (can be accessed via @In private WorldRenderer worldRenderer;). It also holds a reference to the camera.

I would suggest to add a method getPlayerCamera to the world renderer implementation and add this method to the WorldRenderer Interface or a new PlayerCameraProvider Interface. The class already provides cameras so it would fit in the scope. getActiveCamera would do what we want but the name is misleading and the active camera may become something else later on so I suggest an extra method for the player camera.
In the second case of a new interface, we have to update InitialiseRemoteWorld and InitialiseWorld (moar code duplication
sigh) and add the PlayerCameraProvider to the context for dependency injection.
@emanuele3d perefences/comments?

emanuele3d · 2017-09-09T21:27:10Z

I agree with @oniatus that we shouldn't give in on the mess that LocalPlayerSystem is, especially when the camera can be obtained from the WorldRenderer which in turn can be obtained via the Context if @in doesn't work.

Regarding a new method getPlayerCamera():

the method getActiveCamera() -was- called getPlayerCamera() but was eventually renamed in view of eventually having multiple cameras, with only one active (and being rendered) at any given time, presumably all handled within the renderer.

That was a long time ago and the vision now is for render nodes and entire branches of the Render Graph to potentially use multiple cameras, even switch them at runtime, cameras being managed and interacted with through the ES rather than through the renderer.

In this context, in theory I don't disagree with the semantic reasons you mention @oniatus, but in practice the idea is to get rid of even the existing getActiveCamera() method and any other camera-related functionality from the renderer. The renderer will ultimately become a consumer of cameras rather than a provider.

So, adding a new camera-providing method would be temporary and going in the opposite direction from what we are aiming for.

Changes in fov resolution

8860b3d

emanuele3d reviewed Aug 16, 2017

View reviewed changes

emanuele3d suggested changes Aug 17, 2017

View reviewed changes

Cervator assigned 0shine0 Oct 19, 2017

0shine0 closed this Jan 16, 2018

0shine0 deleted the shine2 branch January 16, 2018 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes in fov resolution #3071

Changes in fov resolution #3071

0shine0 commented Aug 16, 2017

GooeyHub commented Aug 16, 2017

emanuele3d Aug 16, 2017 •

edited

Loading

0shine0 Aug 17, 2017

emanuele3d Aug 17, 2017

0shine0 Aug 18, 2017

emanuele3d Aug 18, 2017

emanuele3d Aug 16, 2017

Cervator Aug 16, 2017

0shine0 Aug 17, 2017

emanuele3d Aug 17, 2017

OvermindDL1 Aug 17, 2017 •

edited

Loading

OvermindDL1 Aug 17, 2017

Cervator Aug 18, 2017

emanuele3d Aug 18, 2017

emanuele3d Aug 18, 2017

Cervator Aug 18, 2017

emanuele3d left a comment

emanuele3d Aug 17, 2017

emanuele3d Aug 17, 2017

Cervator commented Sep 6, 2017

0shine0 commented Sep 8, 2017

oniatus commented Sep 8, 2017

0shine0 commented Sep 8, 2017

oniatus commented Sep 8, 2017 •

edited

Loading

0shine0 commented Sep 8, 2017

oniatus commented Sep 8, 2017 •

edited

Loading

emanuele3d commented Sep 9, 2017

Changes in fov resolution #3071

Changes in fov resolution #3071

Conversation

0shine0 commented Aug 16, 2017

GooeyHub commented Aug 16, 2017

emanuele3d Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OvermindDL1 Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emanuele3d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cervator commented Sep 6, 2017

0shine0 commented Sep 8, 2017

oniatus commented Sep 8, 2017

0shine0 commented Sep 8, 2017

oniatus commented Sep 8, 2017 • edited Loading

0shine0 commented Sep 8, 2017

oniatus commented Sep 8, 2017 • edited Loading

emanuele3d commented Sep 9, 2017

emanuele3d Aug 16, 2017 •

edited

Loading

OvermindDL1 Aug 17, 2017 •

edited

Loading

oniatus commented Sep 8, 2017 •

edited

Loading

oniatus commented Sep 8, 2017 •

edited

Loading