Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move object nametags to camera #3712

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@RealBadAngel
Copy link
Contributor

commented Feb 15, 2016

This code moves displaying of nametags out of world geometry. The reasons to do that:

  • texts as world objects cannot write their own depth, they will break all the effects basing on scene depth
  • texts would be shaded, outlined, bumpmapped as everything else without a posibility to provide correct data to do so
  • also, since text scene node is broken for irrlicht 1.8, using it was causing MANY strange bugs: things go randomly opaque etc

My idea is to move them to HUD, its an information for player shown as extra. Turining HUD off would disable them, quite useful when trying to get a screenshot on multiplayer servers...
@est31 is opposing to make them part of HUD, so please share your opinions on that

EDIT: moved them to camera

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2016

Put it in camera.cpp please. It does not belong into hud.cpp.

@RealBadAngel RealBadAngel force-pushed the RealBadAngel:nametag branch Feb 15, 2016

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2016

From wikipedia: A [...] HUD [...] is any transparent display that presents data without requiring users to look away from their usual viewpoints.

So anything that doesn't stay fixed on the screen doesn't belong to HUD. waypoints for example don't belong to HUD at all, same goes for nametags. Basically anything that requires transform matrices based on the player's position or viewing direction. Two ideas for something more suiting:

  1. Put the std::list<Nametag *> into the client class, the drawNametags method goes to drawscene.cpp, and the Nametag definition goes drawscene.h
  2. Put the std::list<Nametag *> together with the Nametag definition and the drawNametags method into camera.cpp, into the Camera class.

I'd prefer 2., but both ways would be okay for me.

I do not oppose the thought to work around apparent bugs in irrlicht (I think you said this fixes #1404, right?) and drawing the stuff ourselves, but object nametags conflict with the very definition of what HUD is about.

👎 until the PR puts object nametag drawing into another class and file.

@RealBadAngel

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2016

I srongly disagree with your definition, have you seen HUDs in airplanes? all those MOVING crosshairs around targets, with names above it?
Fixed is not general attribute of HUD
see for example https://www.youtube.com/watch?v=Ay6g66FbkmQ at about 0:30
But im ok with moving it to camera if others also say so.

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 16, 2016

I agree with moving nametags out of world geometry.
I do think they are inherently a HUD thing, although i don't feel too strongly about this, if other devs insist on the code being somewhere else i'll be okay with that.
I agree nametags should be able to be removed from view and would be happy with them being toggled with HUD.

@RealBadAngel RealBadAngel force-pushed the RealBadAngel:nametag branch Feb 17, 2016

@RealBadAngel RealBadAngel changed the title Move object nametags to HUD Move object nametags to camera Feb 17, 2016

@RealBadAngel RealBadAngel force-pushed the RealBadAngel:nametag branch Feb 17, 2016

@obneq

This comment has been minimized.

Copy link

commented Feb 17, 2016

@est31 the way you insist on nametags not being HUD is very weird...

@est31

View changes

src/camera.cpp Outdated
@@ -80,7 +81,8 @@ Camera::Camera(scene::ISceneManager* smgr, MapDrawControl& draw_control,
m_camera_mode(CAMERA_MODE_FIRST)
{
//dstream<<FUNCTION_NAME<<std::endl;

This comment has been minimized.

Copy link
@est31

est31 Feb 17, 2016

Contributor

trailing whitespace

@est31

View changes

src/camera.cpp Outdated
{
core::matrix4 trans = m_cameranode->getProjectionMatrix();
trans *= m_cameranode->getViewMatrix();

This comment has been minimized.

Copy link
@est31

est31 Feb 17, 2016

Contributor

trailing whitespace

@est31

View changes

src/camera.h Outdated
nametag_text(a_nametag_text),
nametag_color(a_nametag_color)
{
}

This comment has been minimized.

Copy link
@est31

est31 Feb 17, 2016

Contributor

trailing whitespace

@est31

View changes

src/camera.h Outdated
Nametag *addNametag(scene::ISceneNode *parent_node,
std::string nametag_text, video::SColor nametag_color);

void removeNametag(Nametag *nametag);

This comment has been minimized.

Copy link
@est31

est31 Feb 17, 2016

Contributor

trailing whitespace

@RealBadAngel RealBadAngel force-pushed the RealBadAngel:nametag branch Feb 17, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

On IRC >
VanessaE > paramat, RealBadAngel: clip/fade nametags at twice the current view range, but clip the player models right at the range. Makes it easier to follow someone if they keep going in and out of your view range.
14:45 game play implication is none, because the user could just hit 'r' to see distant names if they want, but of course doing that costs massive fps
14:45 (having one player follow another at running speed is quite common in multiplayer, "come look at this thing I made" etc)

I agree with the above.
Please could you add details to this?: what toggles nametags on/off, what distance they are displayed with/without 'R', how they respect 'player transfer distance'
Or does this PR not affect nametag toggle/distance etc.?

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

Added line comments. Very nearly ready to +1 this.

@RealBadAngel

This comment has been minimized.

Copy link
Contributor Author

commented Feb 18, 2016

@paramat toggling is done with F1 as HUD/wielded
i havent changed distance yet. Best idea about that was @VanessaE one i think. Not sure if i should apply it now or with separate commit. Some may have other ideas on range etc and i dont want to delay this PR since there are already two commits made on top of this one.

EDIT: about applying distances i would rather wait for the camera view distance cleanup

@RealBadAngel RealBadAngel force-pushed the RealBadAngel:nametag branch to 0d0eb47 Feb 18, 2016

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

Okay 👍

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2016

I would prefer the old state, with no fade-out of nametags at a distance. Or if there is fade-out, it should be logarithmic to the distance, so that you still can see nametags which are sqrt(3)*32k nodes away (the farthest distance that two players can have in minetest).

@@ -695,14 +699,15 @@ v3f GenericCAO::getPosition()

scene::ISceneNode* GenericCAO::getSceneNode()
{
if (m_meshnode)

This comment has been minimized.

Copy link
@est31

est31 Feb 18, 2016

Contributor

There is no need for this, but okay.

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2016

👍 if tested (paramat, did you test?).

@paramat

This comment has been minimized.

Copy link
Member

commented Feb 18, 2016

I didn't test.

@est31

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2016

Okay, I tested, and it worked. Merged as c3b2797

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.