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

Emscripten support #7510

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@Milek7
Copy link

commented Apr 13, 2019

This is currently WIP, but I would appreciate feedback on it.
Libtimidity is restored with mixer output, Emscripten support is added and various fixes for Emscripten quirks are applied.
Dockerfile for build environment is here: https://github.com/Milek7/openttd-emscripten-dockerfile/tree/sdl1

It currently works on SDL1 on 32bpp blitter (8bpp blitter doesn't work, I suspect broken SDL palette support in emscripten shim). It doesn't look correct though, because html provides BGRA buffer and all blitters assume RGBA. Modifying union Colour works, but this is hacky solution.

It works too on SDL2 branch, with significantly better performance, both on 8bpp (with correct colors) and 32bpp (same BGRA issue, but it is faster than 8bpp). Maybe this PR should wait on SDL2, as it works much better on it.

Network is handled by emscripten as WebSockets, both UDP and TCP. It requires proxy, like this one: https://gist.github.com/Milek7/b921ec9f8d875fe2d5a8b06bfd533834
sockaddr sizes and NetworkAddress comparing needs to be investigated, current workaround is to delete domain name after resolving, and comparing generated hostnames by strcmp. Using original code caused duplicated entries in gamelist as UDP queries received weren't matched to correct entry.

Milek7 added some commits Apr 1, 2019

Codechange: Isolate type macro from math.h in Squirrel code
type macro from Squirrel code clashes with Emscripten math.h.
This is likely Emscripten fault because it shouldn't define own identifiers without __ prefix, but, eh.

@Milek7 Milek7 force-pushed the Milek7:ems1 branch from 3a00b1c to 4976b1d Apr 14, 2019

@glx22

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2019

It's hidden in the commit-checker log but

*** b/src/video/sdl_v.h:56: Use tabs for indentation: '    uint32 cur_ticks;'
*** b/src/video/sdl_v.h:57: Use tabs for indentation: '    uint32 last_cur_ticks;'
*** b/src/video/sdl_v.h:58: Use tabs for indentation: '    uint32 next_tick;'
*** b/src/video/sdl_v.h:59: Use tabs for indentation: '    std::thread draw_thread;'
*** b/src/video/sdl_v.h:60: Use tabs for indentation: '    std::unique_lock<std::recursive_mutex> draw_lock;'
Show resolved Hide resolved src/network/core/address.cpp Outdated

@Milek7 Milek7 force-pushed the Milek7:ems1 branch from 4976b1d to 5ca5c3a Apr 14, 2019

#ifndef __EMSCRIPTEN__
if (err != 0) {
#else
if (err != 0 && errno != EINPROGRESS) {

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

Presumably this is because Emscripten doesn't define EINPROGRESS ? Might be cleaner to define it to something (0 would work?) than the 2 separate if statements

Also add a comment explaining why it's necessary

This comment has been minimized.

Copy link
@Milek7

Milek7 Apr 14, 2019

Author

Emscripten doesn't support synchronous sockets, so they are always created nonblocking. (thus connect always returns EINPROGRESS, and that cannot be interpreted as error)

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 15, 2019

Member

Fair enough, just a comment explaining as such then

if (err != 0) {
#else
if (err != 0 && errno != EINPROGRESS) {
#endif

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

missing /* __EMSCRIPTEN__ */ suffix

also in other places

@@ -242,6 +242,11 @@ SOCKET NetworkAddress::Resolve(int family, int socktype, int flags, SocketList *
strecpy(this->hostname, fam == AF_INET ? "0.0.0.0" : "::", lastof(this->hostname));
}

#ifdef __EMSCRIPTEN__

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

would appreciate comment about why this is necessary

This comment has been minimized.

Copy link
@Milek7

Milek7 Apr 14, 2019

Author

emscripten getaddrinfo fails when given AI_ADDRCONFIG.
reset_hostname is set because comparing addresses doesn't match entries properly, thus workaround is to compare strings returned from getnameinfo

@@ -24,8 +24,8 @@
const char *NetworkAddress::GetHostname()
{
if (StrEmpty(this->hostname) && this->address.ss_family != AF_UNSPEC) {
assert(this->address_length != 0);
getnameinfo((struct sockaddr *)&this->address, this->address_length, this->hostname, sizeof(this->hostname), nullptr, 0, NI_NUMERICHOST);
int size = this->address.ss_family == AF_INET6 ? sizeof(struct sockaddr_in6) : sizeof(struct sockaddr_in);

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

why this change? seems like it should be explained sufficiently and split into its own commit
(and this->address_length removed?)

This comment has been minimized.

Copy link
@Milek7

Milek7 Apr 14, 2019

Author

Emscripten getnameinfo returns error when passed size other than sizeof(sockaddr_in)
emscripten-core/emscripten#7636
It isn't guarded by emscripten specific ifdef as it shouldn't hurt other platforms.

int r = this->GetAddressLength() - address.GetAddressLength();
if (r == 0) r = this->address.ss_family - address.address.ss_family;
if (r == 0) r = memcmp(&this->address, &address.address, this->address_length);
if (r == 0) r = this->GetPort() - address.GetPort();
return r;
#else
return strcmp(GetHostname(), address.GetHostname());

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

explanation please.

also it would be easier to read if the ifndef was inverted, imo

{
uint32 mod;
int numkeys;
Uint8 *keys;

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

weird indentation going on here

@@ -656,55 +657,27 @@ void VideoDriver_SDL::Stop()
}
}

void VideoDriver_SDL::MainLoop()

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

This code shouldn't be moved around while modifying it - difficult to tell what's been changed - should be split into separate commits

@@ -14,8 +14,16 @@

#include "video_driver.hpp"

#ifdef __EMSCRIPTEN__
void em_loop(void *arg);

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

is there a particular reason not to make this a (conditional) class method? would save the duplicate friend definition

This comment has been minimized.

Copy link
@Milek7

Milek7 Apr 14, 2019

Author

It is called by function pointer by emscripten. Is it safe to assume that calling convention would match?

@@ -42,6 +52,12 @@ class VideoDriver_SDL : public VideoDriver {
int PollEvent();
bool CreateMainSurface(uint w, uint h);
void SetupKeyboard();

uint32 cur_ticks;

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

I don't like that these are necessary at all...

@@ -6,7 +6,10 @@

#include <squirrel.h>
#include "sqpcheader.h"
#pragma push_macro("type")

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 14, 2019

Member

comment explaination for this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.