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

Dimensional Height Offset #35

Open
wants to merge 2 commits into
base: releases/1.18.1
Choose a base branch
from

Conversation

Robijnvogel
Copy link
Collaborator

Added distribution of different dimensions over positional audio height range.

  • This means that players that are in different dimensions should not hear each other if they are in a Vanilla dimension and otherwise are rather unlikely to hear each other.
  • DIM0 (Overworld) remains on offset 0 so positional audio for the Overworld does not change.
  • All players must use the same version of the mod for positional audio to work at all in non-Overworld dimensions.
  • Changed version number accordingly.

Added distribution of different dimensions over positional audio height range.
- This means that players that are in different dimensions should not hear each other if they are in a Vanilla dimension and otherwise are rather unlikely to hear each other.
- DIM0 (Overworld) remains on offset 0 so positional audio for the Overworld does not change.
- All players must use the same version of the mod for positional audio to work at all in non-Overworld dimensions.
- Changed version number accordingly.
Copy link
Owner

@zsawyer zsawyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if we should get rid of the “y“ reference in the variable and function names and use height or no designation instead. Because in mumble height is Z axis while in Minecraft it is Y axis. This could lead to confusion when reading the code.

mod/build.gradle Outdated Show resolved Hide resolved
int dimID = game.world.dimension.getType().getId();

switch (dimID) {
case 0:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there maybe an enum that we can use here?
Like
Dimension.overworld.getId()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In MC 1.16 there is an enum that can be used, but this pull request is for MC 1.15

In MC 1.15 there are functions Dimension.isNether and Dimension.isSurfaceWorld however the ID would still be needed to identify the End.
So it is better to leave it this way in my honest opinion.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Now I get it. Please use named constants though -> DIM_ID_NETHER, DIM_ID_END, DIM_ID_OVERWORLD
(maybe even remove the comments because it becomes self-explanatory)

@zsawyer zsawyer added this to the 5.0.0 milestone Feb 6, 2021
@Robijnvogel Robijnvogel changed the title Dimensional Y Offset Dimensional Height Offset Feb 20, 2021
- Since MC and Mumble use a different coordinate for height, use the term Height instead of Y in functions and variables.
- Remove superfluous comments
- Remove superfluous "optimization" for Overworld
- Expanded the height-space a little bit to make the height offset calculation slightly easier to understand.
@zsawyer
Copy link
Owner

zsawyer commented Feb 26, 2021

@magneticflux-
Copy link
Collaborator

The way I did it is definitely super hacky, and could probably be done better. Some issues with it that I see currently:

  • It's purely client-side, which causes a lot of issues with configuration
  • Using hashCode could change on different versions since it uses the Object::hashCode implementation.
    • Also, the hashCode contract doesn't support this use case anyway: "This integer need not remain consistent from one execution of an application to another execution of the same application.

Some things to keep in mind with a new implementation:

  • It should be consistent over versions, mods, JVMs, anything that could differ between players on one server
  • It should support modded dimensions
  • It should be configured on the server-side

@zsawyer
Copy link
Owner

zsawyer commented Feb 28, 2021

It should be consistent over versions, mods, JVMs, anything that could differ between players on one server

We had a look at the 1.15 MC API and it looks like we should use constants for -1, 0, 1 (vanilla Minecraft dimensions).
Other than that starting with 1.16.2 dimensions became a lot more complex:

I received some hints from the forge discord:

World#getDimensionKey
Compare against the constants in World - OVERWORLD, NETHER and END

@zsawyer
Copy link
Owner

zsawyer commented Feb 28, 2021

It should support modded dimensions

@Robijnvogel figured that if we would get a number as an identifier we could use some custom hash function distribution for all other dimension and would distribute them across the height axis.

https://github.com/zsawyer/MumbleLink/pull/35/files#diff-f8536ccf08cf3ea815f4660b932f9ca9bb0bc5a2dc7037042d2b99329d38598aR261

@zsawyer
Copy link
Owner

zsawyer commented Feb 28, 2021

It should be configured on the server-side

I would love to avoid this dependency on this kind of integration. It is more robust and easier to maintain if it is only a client. Could you elaborate on the benefit?

@magneticflux-
Copy link
Collaborator

It should be consistent over versions, mods, JVMs, anything that could differ between players on one server

We had a look at the 1.15 MC API and it looks like we should use constants for -1, 0, 1 (vanilla Minecraft dimensions).
Other than that starting with 1.16.2 dimensions became a lot more complex:

I received some hints from the forge discord:
World#getDimensionKey
Compare against the constants in World - OVERWORLD, NETHER and END

Yeah, those constants have been around for ages but they've been replaced with registry keys now. We can switch on those keys and return an integer like in the past, but that doesn't support modded dimensions as easily.

It should support modded dimensions

@Robijnvogel figured that if we would get a number as an identifier we could use some custom hash function distribution for all other dimension and would distribute them across the height axis.

https://github.com/zsawyer/MumbleLink/pull/35/files#diff-f8536ccf08cf3ea815f4660b932f9ca9bb0bc5a2dc7037042d2b99329d38598aR261

A custom hash function would be a possibility as long as it's consistent across different mod loading setups and JVMs and doesn't have collisions for common values.

Another possibility I was considering was creating an arbitrary (and unique) mapping from world registry keys to integers and syncing that to all clients. That would require server integration though.

It should be configured on the server-side

I would love to avoid this dependency on this kind of integration. It is more robust and easier to maintain if it is only a client. Could you elaborate on the benefit?

The benefit I see is it alleviates responsibility from myself and server admins to help users troubleshoot configuration mismatches. I've handled several issues, both on GitHub and over Discord, where clients had mismatched dimensional offset values.

The other configuration technique in my mod (sending a packet to the client with server info) is server-side, and I haven't had any issues with users of the client having issues connecting, just with server admins not understanding string templating.

@zsawyer
Copy link
Owner

zsawyer commented Feb 28, 2021

Maybe it would be easier if we would provide better server tools: Give them a mumo module similar to bf2 to use.

All that is need then is provide the data in identity in a uniform format.

@zsawyer
Copy link
Owner

zsawyer commented Feb 28, 2021

Maybe it would be easier if we would provide better server tools: Give them a mumo module similar to bf2 to use.

All that is need then is provide the data in identity in a uniform format.

The base version of the module would provide only basic dimension based moving (read dimension from identity -> move to channel).

However it would get a little tricky to support custom aspects like teams/guilds/factions/squads/parties or what ever I cannot think of right now:

  1. client needs to send appropriate information about new aspect
    for the Forge mod this could be done by creating addon-mods which implement
  2. mumo module needs to respond to unprecedented aspect

Edit: Maybe the source module gives a hint on how to solve it.

@magneticflux-
Copy link
Collaborator

I agree, Mumo is definitely the "correct" way to do this. Personally though I just don't want to manage plugins on my Mumble server 😄. I'm also not keen on maintaining a Murmur plugin alongside my mod, so if we can fit Minecraft data into an existing plugin's support, that would be great.

Supporting teams would be pretty difficult I think. I've never used teams, so I don't know what options they support.

We also have to keep in mind the 255-wchar limit on the identity field, which could be filled up by arbitrary dimension and team names. I assume just throwing a special error if that limit is reached would be enough, I doubt many people would reach it anyway.

@zsawyer
Copy link
Owner

zsawyer commented Mar 1, 2021

I somehow forgot about the 255 limit on identity.

The Forge mod code already does validate and throw an error before sending too long data.

@zsawyer
Copy link
Owner

zsawyer commented Mar 1, 2021

I just stumbled upon a request where someone wanted dead people to be muted: https://forums.mumble.info/topic/2375-paid-request-mumble-minecraft-plugin/?do=getNewComment

@Robijnvogel
Copy link
Collaborator Author

@magneticflux-
TeamSpeak. Not Teams.
Teams is not really a consumer VOIP program and certainly not something that gamers would regularly use.

I see that you did a similar Y adjustment to what I did, but I am not quite sure in what range the values of the dimension hash code could be.
If it's a very high value, the accuracy of the positional audio could decrease dramatically.

For as far as I see it, there are two options here and I do not see why we would not support both:

  1. Client sided configuration of a per-dimension identity/context/y-offset.
    This would only really be useful for modpacks where copies of the same configuration file are shared by all users.
    This would not really make sense in a "half of my users use Forge and the other half uses the Fabric version of the mod" situation.
    In MC 1.16 this should probably be a list of pairs of dimension registry names with an offset integer so something like:
{"minecraft:nether", -256},
{"minecraft:surface", 0},
{"minecraft:end", 256},
{"twilight_forest:twilight", 1},
etc.

If y-offset is going to be used, numbers -256 to 256 are allowed and the numbers should just be multiplied by 256 to get the y-offset. 512 dimensions is a lot for preregistered dimensions. I am ignoring dynamic dimension registration like RFTools and Mystcraft for now.

  1. An additional server-sided plugin/mod to synchronize on all clients which identity/context/y-offset should be used is very useful as long as both the Forge and Fabric mod support it.
    Configuration could/should be similar to a possible client-sided solution.

Should the client-sided mods include the server-sided solution to support the Open to LAN functionality? I've used this in combination with a Virtual LAN before, so I think it is a valid use-case.

@zsawyer
Copy link
Owner

zsawyer commented Mar 20, 2021

I am inclined to post-pone this into a release after #41 - I'd first like to get the context unision in. Eventhough both Forge and Fabric mod might provide different height data.

@magneticflux- : is that dimension-based height-offset active? I cannot tell from your code (it looks like it gets set to 0 everytime :?).

@magneticflux-
Copy link
Collaborator

@magneticflux- : is that dimension-based height-offset active? I cannot tell from your code (it looks like it gets set to 0 everytime :?).

That's correct; the dimension-based height offset is disabled (set to 0) by default. It can however be enabled in the configuration. I typically recommend people set up channel switching rather than a dimension offset since it helps Mumble/Teamspeak reduce network traffic, so I don't think there are many people using the feature (I intended it as more of a backup solution for when you don't have access to the Minecraft server).

@zsawyer zsawyer modified the milestones: 5.0.0, 6.0.0 Jul 24, 2021
@zsawyer
Copy link
Owner

zsawyer commented Aug 30, 2021

Ok I have updated to 1.17.
I think we got a little side tracked by identity. Let's first focus on trying to agree on an implementation of the coordinate-offset approach.

  1. Implement client-side only approach
    A. Which value can be used and is by default synchronized across clients?
    B. Which distribution algorithm to use?
  2. For now I think it should be on by default with an option to turn it off.

@zsawyer
Copy link
Owner

zsawyer commented Dec 12, 2021

@magneticflux- can we agree on an approach here? See my previous post.

@magneticflux-
Copy link
Collaborator

magneticflux- commented Dec 12, 2021

I think the idea of distributing the worlds like this is good, but I have two issues:

  1. We should avoid special cases as much as possible, i.e. apply the distribution to vanilla worlds as well. Having special cases will increase complexity over time.
  2. We should prevent partial overlaps by establishing 512-block ranges for each dimension (for compatibility with 1.18 and future height increases)

Additionally, the dimension's raw ID is an incrementing number which means we can get away with using it as an "index" instead of hashing it. It would take 256 dimensions to be registered before they loop back every 256.
Nevermind, I was looking at old code. This doesn't work anymore, see below:

@magneticflux-
Copy link
Collaborator

magneticflux- commented Dec 12, 2021

@Robijnvogel Here's what I found for older versions if you want to backport to 1.15 instead of rebasing to 1.18:

  • 1.15: Use world.getDimension().getType().getRawId(). On Fabric at least this picks up modded dimension IDs properly, I assume Forge does the same.
  • 1.16+: No great option since this was the first time the dimension was able to be loaded from a resourcepack and Mojang didn't bother making a Registry implementation to store them. Instead, the GameJoinS2CPacket just serializes the Identifier instances (they're only used for command autocomplete as well). I think our best-case is to use world.registryKey.hashcode() or, to be really safe, implement our own String/Identifier hashcode.

@magneticflux-
Copy link
Collaborator

magneticflux- commented Dec 17, 2021

@Robijnvogel I started a prototype of such a stable hash function that you can use here: magneticflux-/fabric-mumblelink-mod@3866317

I checked that the three vanilla worlds all result in different values mod 2048. I also chose 2048 so that there would be at least 4 bits of intra-block precision since 32-bit floats have 24 bits of precision. (2^20 / 512 == 2048)

@magneticflux-
Copy link
Collaborator

I'm releasing a 1.18 version of my mod soon, so my proposed world ID hash will become harder to change. I don't plan on backporting to any other versions unless requested, so you're safe to just update directly to 1.18.

@zsawyer zsawyer changed the base branch from QnD-MC_JNA to releases/1.18.1 January 24, 2023 18:02
@zsawyer
Copy link
Owner

zsawyer commented Jan 24, 2023

@magneticflux- can you please explain to me what an example value of world.registryKey.value would return (https://github.com/magneticflux-/fabric-mumblelink-mod/blob/12727324ae9ecfc9c6b0ab5b604e824d43cfffa1/src/main/kotlin/com/skaggsm/mumblelinkmod/client/ClientMumbleLinkMod.kt#L136)?

I cannot relate this to anything other than game.level.dimension() which returns this: "ResourceKey[minecraft:dimension / minecraft:overworld]"

Also note that for me there exists game.player.level.dimension() which seems more appropriate.

zsawyer pushed a commit that referenced this pull request Jan 24, 2023
                    // Make people in other dimensions far away so that they're muted.
                    val yAxisAdjuster = (world.registryKey.value.stableHash % 2048) * config.clientDimensionYAxisAdjust
                    camPos[1] += yAxisAdjuster

/**
 * A stable hash function designed for world IDs.
 * Different clients should be able to run this on the same world ID and get the same result.
 *
 * Based on the `djb2` hash function: [Hash Functions](http://www.cse.yorku.ca/~oz/hash.html)
 */
val Identifier.stableHash: Int
    get() {
        var hash = 5381

        for (c in this.toString()) {
            hash += (hash shl 5) + c.code
        }

        return hash
    }

fixes #35
@zsawyer
Copy link
Owner

zsawyer commented Jan 24, 2023

replaced by #63

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

3 participants