Managing SDL1.2/SDL2.0 on Win32 compiling by MSVC #281

drnovice · 2017-03-01T19:53:31Z

This PR manages full SDL1.2/SDL2.0 support on Windows Systems and MSVC.
PR includes the making screen_magnification and scale_filter as global (extern) variables, to use them out of video driver module.
Press Plus (+) / Minus (-) keys to increase/decrease window size, F11 (or Alt+Enter) to toggle fullscreen. Original ratio resolution will be maintained.
Support to scale2x and hqx scalers are maintaned too.

To correctly compile on MSVC, just read SDL_Win32.txt

* sdl_config/sdl2_config with source.list * graph_config with generate.vbs

… if not set default is /arch:SSE2, this patch need to specify explicitly WITH_SSE2 parameter into Preprocessor Definitions on MSVC.

…ut features on Win32 & MSVC.

drnovice · 2017-03-02T20:10:41Z

projects/generate.vbs

@@ -39,7 +39,7 @@ Sub safety_check(sourcelist_file)

 	' Define regexp
 	Set regexp = New RegExp
-	regexp.Pattern = "#|audio/dsp_sdl.c"
+	regexp.Pattern = "#|audio/dsp_sdl.c|os/thread_sdl.c|video/video_sdl"


Added some voices to excluded files pattern to integrity check on source.list, since they are repeated by #if directive

drnovice · 2017-03-02T20:22:31Z

source.list

+		#else
+			os/thread_win32.c
+		#endif
+	#endif


Added directives to create correctly MSVC Project files (.vcxproj), already activating SDL1.2/SDL2.0 files on project into solution

drnovice · 2017-03-02T20:26:20Z

src/video/scalebit.c

@@ -50,7 +50,7 @@
 static inline void stage_scale2x(void* dst0, void* dst1, const void* src0, const void* src1, const void* src2, unsigned pixel, unsigned pixel_per_row)
 {
 	switch (pixel) {
-#if defined(__x86_64__) || defined(_M_X64) || defined(__SSE2__) || (defined(_M_IX86_FP) && (_M_IX86_FP == 2))
+#if defined(__x86_64__) || defined(_M_X64) || defined(__SSE2__) || (defined(_M_IX86_FP) && (_M_IX86_FP == 2) && defined(WITH_SSE2))


This is strange for my Intel Pentium 2020M processor: on Debug build solution, it can manage SSE2 instructions, but NOT on Release build solution. Of course it's a x64 dual core processor.

Pentium 2020M does of course support SSE2.
http://ark.intel.com/products/71142
You may have issues with your compiler.
_M_IX86_FP == 2 means that MSVC options are set to compile with SSE2.
Just set properly MSVC options to compile without SSE2 it you want to

I know it, but remember that if "/arch:" isn't set, default for x64 is SSE2.
I would let this option to make it more explicit.

maybe you are mixing x64 and x86: x64 CPUs are all SSE2 compatible.

https://msdn.microsoft.com/en-us/library/7t5yh4fd.aspx
"/arch:SSE2
Enables the use of SSE2 instructions. This is the default instruction on x86 platforms if no /arch option is specified."
If compiling x86 default is SSE2 on Visual Studio (even on x64 Systems).

As I said first if you want to compile without SSE2 code, just use the proper MSVC option :
Because the x86 compiler generates code that uses SSE2 instructions by default, you must specify /arch:IA32 to disable generation of SSE and SSE2 instructions for x86 processors.

SSE2 can't be default if nothing is set, if could be problem with Visual Studio compiler.
Again, since x64 processors support SSE2, if you compile x86, default is _M_IX86_FP == 2, therefore /arch:SSE2

Trying to use Not Set:

and see what _M_IX86_FP is valued (equal to 2)...

Hey, I found a really interesting topic:
https://social.msdn.microsoft.com/Forums/vstudio/en-US/e9ee12d7-6d6e-4f80-90a4-35b08f383c3f/illegal-code-generated-by-vc2013-compiler-x64-or-x86-with-sse-instructions?forum=vcgeneral
If you surround scale2x_8_sse2_border(...) with #pragma optimize("", off) / #pragma optimize("", on) problem is fixed.
It seems a real serious bug of MSVC 12 optimizer in Release compiling mode.
If you don't like WITH_SSE2 option, we could do so:

#if defined(_MSC_VER) && _MSC_VER == 1800 #pragma optimize("", off) #endif static inline void scale2x_8_sse2_border(scale2x_uint8* dst, const scale2x_uint8* src0, const scale2x_uint8* src1, const scale2x_uint8* src2, unsigned count) { [...] } #if defined(_MSC_VER) && _MSC_VER == 1800 #pragma optimize("", on) #endif

is scale2x_8_sse2_border() the only function affected by the MSVC 2012 bug ?

Currently it seems it was only this function (I've got VS2013 Community, v12 release: I cannot assess if it happens even on VS2012). Anyway I tested all filters (nearest, scale2x & hqx) both debug/release compilations, and no other compiler/optimizer bugs detected.
Now it's good for FreeBSD compiling? Can we accept this PR?

drnovice · 2017-03-02T20:33:10Z

src/input/mouse.c

+#if defined(_WIN32) && defined(WITH_SDL2)
+	if (g_scale_filter == FILTER_NEAREST_NEIGHBOR) {
+		g_mouseX = mouseX / g_screen_magnification;
+		g_mouseY = mouseY / g_screen_magnification;


It's necessary since SDL_RenderSetLogicalSize is no more used: it seems can't manage very well cursor pointer x/y limits. Alternatively SDL_RenderCopy is managed by Dst Rect.

drnovice · 2017-03-03T09:54:06Z

src/opendune.c

-	if (!Video_Init(screen_magnification, filter)) return false;
+#if !(defined(_WIN32) && (defined(WITH_SDL) || defined(WITH_SDL2)))
+	if (!Video_Init()) return false;
+#endif


Since SDL_Init under Windows must be used in same thread of Video_Tick, where inputs are recognized, I added SDL_Init call inside Video_Tick with Win32.

drnovice · 2017-03-03T09:57:10Z

src/opendune.c

 	Video_Uninit();
+#endif


Same for SDL_Quit into Video_Uninit (see comment above)

drnovice · 2017-03-03T10:23:45Z

src/opendune.c

@@ -1155,9 +1155,14 @@ static bool OpenDune_Init(int screen_magnification, VideoScaleFilter filter, int
 		return false;
 	}

+	g_screen_magnification = screen_magnification;
+	g_scale_filter = filter;
+


Turned "screen_magnification" and "scale_filter" parameters into global extern variables, to can use them freely inside Video_Tick and Video_Init calls, since we added internal SDL_Init call into Video_Tick thread (see comments about Video_Init).

miniupnp · 2017-03-03T17:10:12Z

well it doesn't even compile... :(
on my FreeBSD machine with sdl2. with SDL1 it's fine

miniupnp · 2017-03-03T17:14:36Z

also, please note that SSE2 is part of the AMD64 / x64 specification

drnovice · 2017-03-04T10:09:02Z

What is the problem about your compiler on FreeBSD?

miniupnp · 2017-03-06T09:32:51Z

[SRC] Compiling video/video_sdl2.c
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c: In function 'Video_DrawScreen_Scale2x':
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c:388:2: warning: ISO C90 forbids mixed declarations and code [-Wpedantic]
  int render_width = SCREEN_WIDTH * g_screen_magnification;
  ^
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c: In function 'Video_DrawScreen_Hqx':
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c:440:2: warning: ISO C90 forbids mixed declarations and code [-Wpedantic]
  int render_width = SCREEN_WIDTH * g_screen_magnification;
  ^
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c: In function 'Video_DrawScreen':
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c:498:2: warning: ISO C90 forbids mixed declarations and code [-Wpedantic]
  SDL_Rect Src, Dest;
  ^
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c: In function 'Video_ToggleFullscreen':
/home/nanard/code/git/OpenDUNE/src/video/video_sdl2.c:545:3: error: expected expression before '/' token
   //SDL_SetWindowFullscreen(s_window, 0);
   ^

miniupnp · 2017-03-06T09:41:07Z

I just had a quick look at video_sdl2.c, I don't understand why you changed the fullscreen part.
The original worked better (tested on FreeBSD 10)

drnovice · 2017-03-06T19:57:17Z

Errors are due to some // comments (c++ style), warnings are because I declared some int vars after some operations in those functions...
Even if I can't understand why those comments:
[..]\OpenDUNE\src\video\hqx.c(64): // Load image
[..]\OpenDUNE\src\video\hqx.c(73): // Allocate memory for image data
[..]\OpenDUNE\src\video\hqx.c(79): // Init srcData from loaded image
[..]\OpenDUNE\src\video\hqx.c(80): // We want the pixels in BGRA format so that when converting to uint32_t
[..]\OpenDUNE\src\video\hqx.c(81): // we get a RGB value due to little-endianness.
[..]\OpenDUNE\src\video\hqx.c(87): // If big endian we have to swap the byte order to get RGB values
[..]\OpenDUNE\src\video\hqx.c(109): // If big endian we have to swap byte order of destData to get BGRA format
[..]\OpenDUNE\src\video\hqx.c(117): // Copy destData into image
[..]\OpenDUNE\src\video\hqx.c(120): // Free image data
[..]\OpenDUNE\src\video\hqx.c(124): // Save image
[..]\OpenDUNE\src\video\hqx.c(125): ilConvertImage(IL_BGRA, IL_UNSIGNED_BYTE); // No alpha channel
don't cause errors, I'll fix all of them (by /* */ c style of course) with a new commit.

drnovice · 2017-03-06T20:08:58Z

I changed fullscreen part to let to use Zoom In/Out features (+/- keys), with integer scaler so as to maintain original aspect ratio. What would you clarify? I left some comments into review changes to explain some of logics. You could reply to one of them if something isn't clear or disagree.

drnovice · 2017-03-06T20:15:03Z

src/video/video_sdl.c

+	} else if (g_scale_filter != FILTER_HQX) {
+		//SDL_SetPalette(s_gfx_surface, SDL_LOGPAL | SDL_PHYSPAL, colors, 0, 256);
+		if (!SDL_SetPalette(s_gfx_surface, SDL_LOGPAL | SDL_PHYSPAL, colors, 0, 256)) Warning("SDL_SetPalette() failed while resizing\n");
+	}


Video_Resize function: reset again surface with new dimensions (width & height for their integer scaler multiplier), recalculating palette if filter isn't HQX, otherwise bad side effect about ingame strange colours.

drnovice · 2017-03-06T20:32:13Z

src/video/video_sdl.c

+	if (!s_fullscreen) {
+		s_prev_magnification = g_screen_magnification;
+		g_screen_magnification = 2;
+


Here we fix magnification to 2 because of HQX filter breaks surface pixels filling, and using a real fullscreen mode it can get better aspect ratio dimensions from 2x instead of from 3x.

drnovice · 2017-03-06T20:43:19Z

src/video/video_sdl.c

+		g_screen_magnification = s_prev_magnification;
+		Video_Resize();
+	}
+#endif


Workaround: this because of SDL1.2 under Windows, when you Quit the game in fullscreen mode, it freezes. So we first return on windowed mode, then can dispose and close all.

drnovice · 2017-03-06T20:48:55Z

src/video/video_sdl2.c

@@ -1,5 +1,8 @@
 /** @file src/video/video_sdl2.c SDL 2 video driver. */

+#if defined(_WIN32)
+#include <stdio.h>
+#endif


Win32 needs stdio.h library.

drnovice · 2017-03-06T20:51:48Z

src/video/video_sdl2.c

@@ -154,6 +159,9 @@ static void Video_Mouse_Move(uint16 x, uint16 y)
 	if (s_mouseMinY != 0 && ry < s_mouseMinY) ry = s_mouseMinY;
 	if (s_mouseMaxY != 0 && ry > s_mouseMaxY) ry = s_mouseMaxY;

+	if (ry > SCREEN_HEIGHT * g_screen_magnification - 1) ry = SCREEN_HEIGHT * g_screen_magnification - 1;
+	if (rx > SCREEN_WIDTH * g_screen_magnification - 1) rx = SCREEN_WIDTH * g_screen_magnification - 1;
+


This checks mouse x/y limits to bottom and right, for fake Fullscreeen_Desktop mode.

drnovice · 2017-03-06T20:54:31Z

src/video/video_sdl2.c


 	if (err != 0) {
 		Error("Could not set logical size: %s\n", SDL_GetError());
 		return false;
-	}
+	}*/


No more necessary RenderSetLogicalSize, since we're using Src/Dst rectangles to RenderCopy, who better manages original aspect ratio.

drnovice · 2017-03-09T21:33:45Z

First to change PR with the fix, I just wish to know why on Ubuntu 16.04 TLS I can't compile opendune:
#283

drnovice · 2017-03-17T19:40:48Z

Tried under Ubuntu 14.04 TLS 32bit (to compile SDL1.2, it needs to add -msse option on configure.lib), and Ubuntu 16.04 TLS 64bit (to compile just remove -ansi and -pedantic option, as suggest by @miniupnp: #283)

miniupnp · 2017-03-24T15:38:32Z

I'm trying to build with SDL under Win32 with MS Visual C++ 2010 but it still includes video_win32.c
how are we supposed to tell to generate.vbs that we are building with SDL or SDL2 ?

drnovice · 2017-03-27T10:29:37Z

Just set:
graph_config = "SDL" or graph_config = "SDL2"
and launch vbs to create correct including from build.

Warning: For SDL2 there is a bug on script creation vbs, line 166, because of double definition in source.list of os/thread_sdl.c (first time it exclude it if not SDL, second time it has to include it if SDL2:

[...]
#if WIN32
	os/error_win32.c
	#if SDL
		os/thread_sdl.c
	#else
		#if SDL2
			os/thread_sdl.c
		#else
			os/thread_win32.c
		#endif
	#endif
	os/readdir_win32.c
#else
[...]

Change:

source_list.Add line, "included"

to:

If source_list.Exists(line) Then
	source_list.Item(line) = "included"
Else
	source_list.Add line, "included"
End If

I'll add vbs fix into this PR.

…sdl_main macro to Win32 SDL compiling

drnovice · 2017-03-27T20:36:50Z

Ok, tested on VS2010, VS2013, VS2015... no other Linux impacts: Let's accept this PR!

drnovice · 2017-03-27T20:39:34Z

src/opendune.c

@@ -21,6 +21,9 @@
 #include <time.h>
 #if defined(WITH_SDL) || defined(WITH_SDL2)
 #include <SDL.h>
+#ifdef _WIN32
+#undef main


This will avoid to have to set /SUBSYSTEM:CONSOLE or /SUBSYSTEM:WINDOW on VS Linker System options...

miniupnp · 2017-05-05T18:09:34Z

Just set:
graph_config = "SDL" or graph_config = "SDL2"
and launch vbs to create correct including from build.

You mean "edit generate.vbs at line XX, then launch it"
That should be explained in SDL_Win32.txt

miniupnp · 2017-05-05T18:19:38Z

OK, I have tested with both SDL 1.2.5 and 2.0.5 under Win32 (32bits) with VC++ 2010 Express.

drnovice · 2017-05-08T15:30:40Z

Ok I updated SDL_Win32.txt
Tests are ok? Can we accept this PR?

miniupnp · 2017-05-09T10:24:47Z

I still have issues with Full screen SDL2 FreeBSD.

drnovice · 2017-05-09T20:26:35Z

What are exactly fullscreen issues?

drnovice · 2017-07-10T08:03:58Z

Does anyone follow this project yet?!?

miniupnp · 2017-11-03T20:01:32Z

I picked up changes that broke nothing for other OS.
ie only support for SDL / SDL2 under windows.

drnovice · 2017-11-05T14:21:13Z

No zoom changes?
I can't understand: what code broke others os?
I tested on Ubuntu 14 32bit (ok), Ubuntu 16 64bit (ok), what is problem on FreeBSD?
Can you show me some screenshot or more info about it?

miniupnp · 2017-11-05T23:34:04Z

I will add zoom change later.

drnovice · 2017-11-06T08:41:34Z

I'll try to install FreeBSD on vm. What version did you use?

drnovice added 4 commits February 28, 2017 11:58

Manage SDL1.2/SDL2.0 choice to project recreation by config parameters:

4740a8a

* sdl_config/sdl2_config with source.list * graph_config with generate.vbs

Since not all x64 processors support SSE2 (e.g. my Pentium 2020M) and…

7c621e9

… if not set default is /arch:SSE2, this patch need to specify explicitly WITH_SSE2 parameter into Preprocessor Definitions on MSVC.

Managing SDL1.2/SDL2.0 configs to use fullscreen and window zoom in/o…

018b1ac

…ut features on Win32 & MSVC.

SDL2: Removing redundant code on drawing methods when doing RenderCopy.

ce29305

drnovice commented Mar 2, 2017

View reviewed changes

drnovice commented Mar 3, 2017

View reviewed changes

src/opendune.c

Video_Uninit();

#endif

Copy link

Contributor Author

drnovice Mar 3, 2017

Choose a reason for hiding this comment

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

Same for SDL_Quit into Video_Uninit (see comment above)

drnovice commented Mar 3, 2017

View reviewed changes

drnovice commented Mar 6, 2017

View reviewed changes

drnovice force-pushed the master branch from 622d9c5 to 018b1ac Compare March 17, 2017 19:03

Fixing compiling errors and warnings on Linux systems

c3284f9

drnovice added 2 commits March 27, 2017 22:31

Fixing generate.vbs, turn off VS optimizer for scale2x_8_sse2, undef …

1e2074c

…sdl_main macro to Win32 SDL compiling

Removing WITH_SSE2 useless option

d24827e

drnovice commented Mar 27, 2017

View reviewed changes

drnovice mentioned this pull request May 3, 2017

Fullscreen instead of a window on startup? #282

Closed

Adding info for generate.vbs settings to use SDL on VS project files

26e0ef6

miniupnp closed this in a30a8cc Nov 3, 2017

Managing SDL1.2/SDL2.0 on Win32 compiling by MSVC #281

Managing SDL1.2/SDL2.0 on Win32 compiling by MSVC #281

Conversation

drnovice commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drnovice Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drnovice Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniupnp commented Mar 3, 2017 • edited

miniupnp commented Mar 3, 2017

drnovice commented Mar 4, 2017

miniupnp commented Mar 6, 2017

miniupnp commented Mar 6, 2017 • edited

drnovice commented Mar 6, 2017

drnovice commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drnovice commented Mar 9, 2017

drnovice commented Mar 17, 2017 • edited

miniupnp commented Mar 24, 2017

drnovice commented Mar 27, 2017

drnovice commented Mar 27, 2017

Choose a reason for hiding this comment

miniupnp commented May 5, 2017

miniupnp commented May 5, 2017

drnovice commented May 8, 2017

miniupnp commented May 9, 2017

drnovice commented May 9, 2017

drnovice commented Jul 10, 2017

miniupnp commented Nov 3, 2017

drnovice commented Nov 5, 2017

miniupnp commented Nov 5, 2017

drnovice commented Nov 6, 2017

drnovice Mar 2, 2017 •

edited

drnovice Mar 2, 2017 •

edited

miniupnp commented Mar 3, 2017 •

edited

miniupnp commented Mar 6, 2017 •

edited

drnovice commented Mar 17, 2017 •

edited