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

Add a FOV field to the mumble link's identity blob #14

Closed
wants to merge 1 commit into from

Conversation

lye
Copy link
Contributor

@lye lye commented Feb 28, 2015

Basically what it says on the tin.

Note that this is a proposed potentially breaking change -- please let me know if this will cause issues with any existing applications (Mumble scripts or otherwise). This really isn't the right place to put the FOV, but the Mumble link data structure is exceptionally limited and this is the only place that won't break everything horribly. Eventually one day in the far-flung future I'd like to have a better transport for this data, but for now this is what we've got.

If this will break your applications/integrations/anything, please comment. I really don't like breaking things.

@codemasher
Copy link
Contributor

On a side note: will the world_id be fixed so that we get a reliable value from it? You can drop it otherwise since the Megaservers messed it up.

@aRestless
Copy link

Markers relies on the NPAPI Mumble Link Plugin for Overwolf by @meh ... I don't know if he's planning to update it.

@meh
Copy link

meh commented Feb 28, 2015

What needs updating?

@aRestless
Copy link

@meh: They're currently thinking about adding a field to the identity blob. And it seems this could break existing implementations. But tbh I don't know how your plugin works internally and how it would be affected by that.

@meh
Copy link

meh commented Feb 28, 2015

@aRestless it wouldn't, it's just JSON, the plugin doesn't care about what's in the fields.

@aRestless
Copy link

@meh Ah okay, thank you for that.

@lye So this means existing implementations would only break if they don't parse the JSON properly? If that's so I think you shouldn't worry too much.

And, as @codemasher already pointed out, a fix on the world_id would be great. I also put some additional selfish wishes into the forum thread, but I think this particular PR isn't the right place for them.

@Archomeda
Copy link
Contributor

As more people have said, if applications are properly coded, adding something to that JSON shouldn't have any complications for existing applications. I would only be concerned if you change the context (because this is binary data without version checks), change the ids within that JSON or exceed the 256 wide character limit somehow.

This really isn't the right place to put the FOV, but the Mumble link data structure is exceptionally limited and this is the only place that won't break everything horribly. Eventually one day in the far-flung future I'd like to have a better transport for this data, but for now this is what we've got.

It's a far-flung future I look forward to then :) Just a suggestion, but is implementing your own memory mapped file structure next to the existing Mumble Link an idea? Of course with versioning that Mumble Link seems to be doing as well. This way you have control of the struct yourself (including the size).

@lye
Copy link
Contributor Author

lye commented Mar 1, 2015

And, as @codemasher already pointed out, a fix on the world_id would be great.

I'll look into fixing that. Thought it was just a weird value from my local environment.

My main concern w.r.t. adding to the identity field was that some script somewhere relied on it being immutable for the course of a session. After thinking about it some more, I realized the commander status is already mutable so I'm much less concerned about breakage.

I'm somewhat miffed that the FOV isn't an included field with the camera data -- since it basically lets you compute the player's view frustum -- but it makes sense to include in the link data.

@aRestless
Copy link

@lye Well, if you really wanted to leave the identity field alone, you could put the fov in the camera's top vector, since it's absolute value is meaningless and (currently) never changes. But, well, that's as nasty as it gets.

@Archomeda
Copy link
Contributor

My main concern w.r.t. adding to the identity field was that some script somewhere relied on it being immutable for the course of a session. After thinking about it some more, I realized the commander status is already mutable so I'm much less concerned about breakage.

I see what you mean.

For reference: It's the same with e.g. map_id, team_color_id and name. I assume that because of those values, most people won't think it's immutable for the course of a session instinctively (which I assume you are talking about a complete game session?). Also, the Mumble Wiki doesn't specify it as totally immutable, just as "don't update it too frequently".

I'm somewhat miffed that the FOV isn't an included field with the camera data -- since it basically lets you compute the player's view frustum -- but it makes sense to include in the link data.

I doubt they had use for it in their positional audio or didn't think about it when they made this format almost 5.5 years ago (not sure, just guessing).

Anyway, I can't think of any case where getting the FoV can be useful myself, but I'm pretty sure others will (then again, I mostly use this API for getting data from the identity only). It looks good to me for when the camera update arrives.

@Remfin
Copy link

Remfin commented Mar 9, 2015

@lye Before you go "fixing" "world_id", you may want to verify it's actually wrong. I'm pretty sure people don't understand what it actually is, which is an identifier for that specific instance of the map (Cliff called it a shardId). At least, I verified two players on the same map/same instance had the same value after Megaservers, and I doubt anything would have broken since then...

@Archomeda
Copy link
Contributor

@Remfin I would argue it is actually wrong. People would expect world_id to be the id of their own server. The reason why, is that it always has shown the server id (before the mega servers, except overflows, which most people took as a bug).

Even if world_id gets fixed to show the proper world id, it is still possible to get the shardId. Take a look at the MumbleContext, shardId is defined there.

@Remfin
Copy link

Remfin commented Mar 10, 2015

@Archomeda Sorry, I had totally forgotten it was duplicated in the text area too :) It still means what it always meant though, it was just poorly named.

If anything is going to change it about it (which would be a breaking change), they should probably just get rid of it and reclaim some of that precious text space. It's not like in the reality of Megaservers that a "world id" means anything...it only exists as an (invisible) input into the formula for routing you to a map instance.

@lye
Copy link
Contributor Author

lye commented Mar 10, 2015

It still means what it always meant though, it was just poorly named.

Ehh, it was correct before megaservers (e.g., it was actually the world id). I'd like to see it be made correct again, though I haven't been able to get it working yet :<

@lye
Copy link
Contributor Author

lye commented Mar 10, 2015

Also, it's too late for me to back out the FOV change so I'm just going to close this PR <3

I'll make a new one for the world_id field fix at some point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants