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

[WIP] Add support for double precision floats #288 #12299

Closed
wants to merge 1 commit into from

Conversation

fire
Copy link
Member

@fire fire commented Oct 22, 2017

Work in progress. DO NOT MERGE.

See: #288

@fire
Copy link
Member Author

fire commented Oct 22, 2017

1>  cl /Fothirdparty\libpng\pngrtran.windows.opt.tools.64.obj /c thirdparty\libpng\pngrtran.c /nologo /O2 /DDEBUG_ENABLED /MT /Gd /GR /nologo "/IC:\Program Files (x86)\Windows Kits\10\/Include" /DWINDOWS_ENABLED /DOPENGL_ENABLED /DRTAUDIO_ENABLED /DWASAPI_ENABLED /DTYPED_METHOD_BIND /DWIN32 /DWINVER=0x0601 /D_WIN32_WINNT=0x0601 /D_WIN64 /DREAL_T_IS_DOUBLE "/IC:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.11.25503\include" "/IC:\Program Files (x86)\Windows Kits\NETFXSDK\4.6.1\include\um" "/IC:\Program Files (x86)\Windows Kits\10\include\10.0.15063.0\ucrt" "/IC:\Program Files (x86)\Windows Kits\10\include\10.0.15063.0\shared" "/IC:\Program Files (x86)\Windows Kits\10\include\10.0.15063.0\um" "/IC:\Program Files (x86)\Windows Kits\10\include\10.0.15063.0\winrt" "/IC:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\include" /I "/IC:\Program Files (x86)\Microsoft Visual Studio 14.0\VC\atlmfc\include" /I "/IC:\Program Files (x86)\Windows Kits\10\Include\10.0.10240.0\ucrt" /I /I "/IC:\Program Files (x86)\Windows Kits\8.1\Include\um" "/IC:\Program Files (x86)\Windows Kits\8.1\Include\shared" "/IC:\Program Files (x86)\Windows Kits\8.1\Include\winrt" /I /I /w /DMSVC /GR -DPTRCALL_ENABLED -DTOOLS_ENABLED -DGDSCRIPT_ENABLED -DMINIZIP_ENABLED -DXML_ENABLED -DGLAD_ENABLED -DGLES_OVER_GL -DPNG_ARM_NEON_OPT=0 /Icore /Icore\math /Ieditor /Idrivers /I. /Iplatform\windows /Ithirdparty\zstd /Ithirdparty\zstd\common /Ithirdparty\zlib /Ithirdparty\rtaudio /Ithirdparty\glad /Ithirdparty\libpng
1>C:\Program Files (x86)\Microsoft Visual Studio\2017\Professional\VC\Tools\MSVC\14.11.25503\include\type_traits(2342): warning C4577: 'noexcept' used with no exception handling mode specified; termination on exception is not guaranteed. Specify /EHsc
1>  pngmem.c
1>drivers\gles3\shader_gles3.cpp(98): error C2664: 'void (GLint,GLsizei,GLboolean,const GLfloat *)': cannot convert argument 4 from 'const real_t *' to 'const GLfloat *'
1>  drivers\gles3\shader_gles3.cpp(98): note: Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or function-style cast
1>  pngpread.c

Blocked until I investigate this futher.

@fire
Copy link
Member Author

fire commented Oct 22, 2017

https://www.khronos.org/registry/OpenGL/extensions/ARB/ARB_gpu_shader_fp64.txt Need to get the fp64 functions somehow.

@akien-mga akien-mga changed the title Add support for double precision floats #288 [WIP] Add support for double precision floats #288 Oct 22, 2017
@fire fire force-pushed the double branch 2 times, most recently from 0bb8578 to 7026033 Compare October 22, 2017 17:41
@fire
Copy link
Member Author

fire commented Oct 27, 2017

My plan is to split the patch into converting floats to real_t and @akien-mga What do you think?

That means trying to do the first #288 (comment) .

  1. There is some support for using doubles in binary serialization,
    but it's probably not complete. files such as resouce_format_binary.cpp
    and marshalls.cpp would need to be checked and made sure that
    doubles are written (and read) in a binary compatible way.

This is tested via float=64 option. Currently failing.

@akien-mga
Copy link
Member

Thanks a lot for your contribution!
We are now entering a strict release freeze for Godot 3.0 and will only consider major bug fixes. We won't merge new features and enhancements anymore until 3.0 is released.

Moving this PR to the 3.1 milestone, to be reviewed once the release freeze is lifted. It could eventually be cherry-picked for a future 3.0.1 maintenance release if it doesn't change the user-facing APIs and doesn't compromise the engine's stability.

@akien-mga akien-mga added this to the 3.1 milestone Jan 4, 2018
@aaronfranke
Copy link
Member

@akien-mga This should definitely be considered a feature request and not a bug fix. It doesn't make too much sense to add to a 3.0.1 maintenance release, but the 3.1 milestone does make a lot of sense.

@reduz
Copy link
Member

reduz commented Jan 15, 2018 via email

@fire
Copy link
Member Author

fire commented Apr 29, 2018

Was able to get a build running with many things converted to real_t (double).

Use scons p=windows vsproj=yes float=64 -j36 to build with floating point being 64 bit.

I had to disable ptrcall, bullet and gdnative. Would be interested in people looking why they break.

Note that the graphics driver still converts back to float.

@fire
Copy link
Member Author

fire commented Apr 29, 2018

Hm, it was actually having linking errors :/ However if someone could help that would be so useful.

@aaronfranke
Copy link
Member

aaronfranke commented Apr 29, 2018

Color math can be left as single-precision if it doesn't cause any issues. 23 significant digits is enough for HDR color math (should have 16 bits per channel) and most displays have 8-12 bits per channel.

@aaronfranke
Copy link
Member

aaronfranke commented Jul 8, 2018

@fire I think it would be best if you created many smaller pull requests instead of this one big pull request. Not only does it get conflicts easily but also it's hard to review so many changes.

For example, you could make pull requests for "replace float with real_t in x and y files" or something. If you did this then we could merge these small pull requests one at a time to slowly bring the engine to be double-compatible and also have others test each small change as it gets into master.

@fire
Copy link
Member Author

fire commented Jul 23, 2018

Got your message, I'll try to find effort to work on this.

@@ -629,14 +629,14 @@ uint32_t BulletPhysicsServer::body_get_user_flags(RID p_body) const {
return 0;
}

void BulletPhysicsServer::body_set_param(RID p_body, BodyParameter p_param, float p_value) {
void BulletPhysicsServer::body_set_param(RID p_body, BodyParameter p_param, real_t p_value) {
Copy link
Member

@aaronfranke aaronfranke Sep 14, 2018

Choose a reason for hiding this comment

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

Shouldn't these changes

  1. Be using Bullet's btScalar instead of Godot's real_t?

  2. Be contributed upstream to Bullet directly?

Copy link
Member

Choose a reason for hiding this comment

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

modules/bullet is Godot's code, not upstream.

@aaronfranke
Copy link
Member

This PR should probably be assigned the "salvageable" tag.

@fire
Copy link
Member Author

fire commented Apr 6, 2019

@aaronfranke What are your plans for taking this forward? Doing some spring updates and cleaning on my pull requests.

@aaronfranke
Copy link
Member

After #21922 is merged I plan to pick-and-test parts of this one, since this PR is functional-ish.

@fire
Copy link
Member Author

fire commented Apr 30, 2019

@aaronfranke Updated to lastest. Still work in progress.

@Anutrix
Copy link
Contributor

Anutrix commented Jun 18, 2019

Any updates?

@fire
Copy link
Member Author

fire commented Jul 12, 2019

Rebased.

@akien-mga
Copy link
Member

Moving to 4.0 milestone as that's when we'll handle #288 (via this PR, #21922 and some more changes needed).

@fire
Copy link
Member Author

fire commented Oct 22, 2019

This pull request is very out of date.

I'll take a look at rebasing on top of #21922 and Vulkan.

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.

5 participants