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

Fix render pipeline lighting issue with small cubes #8158

Merged
merged 3 commits into from Nov 25, 2021

Conversation

sekwah41
Copy link
Contributor

@sekwah41 sekwah41 commented Oct 17, 2021

This will also allow other mods which need to calculate small normalised vectors to be able to use the default classes without needing to write their own, while not affecting any side effects created by how the base game uses this function.

image

It fixes the bug shown above and is useful for other mods where any problems with the resulting vector are probably not as bad as it not being calculated in the first place.

After changes
image

This will also allow other mods which need to calculate small normalised vectors to be able to use the default classes without needing to write their own, while not effecting any side effects created by how the base game uses this function.
@marchermans
Copy link
Contributor

So I know that this fixes it, since this is how I fixed this in C&B.
But would this not also require a Mojang Minecraft Issue ticket?

@sekwah41
Copy link
Contributor Author

sekwah41 commented Oct 17, 2021

I'm don't know if it creates a problem in vanilla. So far I've only found it only causes a problem in the experimental forge render pipeline.

I'm not sure how quick or if mojang would fix it unless a vanilla problem is found.

@gigaherz
Copy link
Contributor

gigaherz commented Oct 17, 2021

If the bug is in vanilla code we request that the issues are reported to mojang. Even if it's unlikely they will fix them, we want them to be reported.

We can then mark forge's fix as // Forge: fix MC-xxxxx

@sekwah41
Copy link
Contributor Author

If the bug is in vanilla code we request that the issues are reported to mojang. Even if it's unlikely they will fix them, we want them to be reported.

We can then mark forge's fix as // Forge: fix MC-xxxxx

I'll report it when I'm back later then. Though I'll take a quick look at where this function is used in vanilla to see if I can find anything it might break.

@sciwhiz12 sciwhiz12 added 1.17 Bug This request reports or fixes a new or existing bug. Rendering This request deals with rendering. These must be treated with care, and tend to progress slowly. labels Oct 17, 2021
@sekwah41
Copy link
Contributor Author

I've created the issue https://bugs.mojang.com/browse/MC-239212

I will also change the code to just avoid it when the length is 0 rather than avoiding it entirely, seeing as normalising doesn't work in that case anyway.

Allows to still check for a 0 length vector. After checking what its used for, nowhere uses the return boolean it seems and its its only ever large vectors or rendering.
@gigaherz
Copy link
Contributor

I'm wondering if it would be worth doing instead, if ((double)f < Float.MIN_NORMAL) which would mean the value is either denormalized or zero.

@pupnewfster
Copy link
Member

I'm wondering if it would be worth doing instead, if ((double)f < Float.MIN_NORMAL) which would mean the value is either denormalized or zero.

At that point shouldn't it be if (f < Float.MIN_NORMAL) so as to not potentially have issues due to float to double floating point precision? At least given the fact it seems to use it as a float still lower down so wouldn't have that precision issue show up there instead.

float f = this.f_122228_ * this.f_122228_ + this.f_122229_ * this.f_122229_ + this.f_122230_ * this.f_122230_;
- if ((double)f < 1.0E-5D) {
+ if (!force && (double)f < 1.0E-5D) {
+ if ((double)f == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

At the end of this line you should include a comment like: //Forge: Fix MC-239212

@sekwah41
Copy link
Contributor Author

sekwah41 commented Oct 17, 2021

I'm wondering if it would be worth doing instead, if ((double)f < Float.MIN_NORMAL) which would mean the value is either denormalized or zero.

Forgot that existed, that would probably be the best

@sekwah41
Copy link
Contributor Author

sekwah41 commented Oct 19, 2021

After taking a deeper look into the code, the only time Mojang ever calls it, they actually perform another function on it which doesn't seem to matter if it's normalised or not.

Other than that they normalise some vectors which are pre-defined.

I hope they fix it but with that in mind, altering this and making it part of forge should not cause any problems in mc and should only benefit any mods using these classes rather than implementing their own.

@ChampionAsh5357 ChampionAsh5357 added the Assigned This request has been assigned to a core developer. Will not go stale. label Nov 24, 2021
@gigaherz gigaherz merged commit 1112819 into MinecraftForge:1.17.x Nov 25, 2021
@sekwah41 sekwah41 deleted the fix-pipeline-block-issue branch November 26, 2021 12:23
DarkEyeDragon pushed a commit to DarkEyeDragon/MinecraftForge that referenced this pull request Dec 26, 2021
)

This will also allow other mods which need to calculate small normalised vectors to be able to use the default classes without needing to write their own, while not effecting any side effects created by how the base game uses this function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.17 Assigned This request has been assigned to a core developer. Will not go stale. Bug This request reports or fixes a new or existing bug. Rendering This request deals with rendering. These must be treated with care, and tend to progress slowly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants