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

Nintendo Switch Port #1067

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

WinterMute
Copy link
Contributor

This isn't quite ready for merging. I wanted to start the PR and get some advice on how best to approach some of this.

I'm not entirely keen on this kind of construction - https://github.com/WinterMute/chocolate-doom/blob/b89a85f15418bc6046b7ce7460fe54cdfa5919d2/src/Makefile.am#L12-L24. Would it perhaps be better to have something like an --enable-setup option for this?

For some reason use_joystick seems to not be bound in heretic and I can't really figure out why atm. For now I've just defaulted the joystick to enabled on Switch here - https://github.com/WinterMute/chocolate-doom/blob/b89a85f15418bc6046b7ce7460fe54cdfa5919d2/src/i_joystick.c#L43-L49

I have some more testing to do but so far it all seems to work quite well on hardware & I have some binaries uploaded @ https://github.com/WinterMute/chocolate-doom/releases/tag/switch-port-1.0.0-alpha

I'm also having trouble getting Strife to load its shareware wad & I see there's a fixme comment in the wad list for Strife. Any ideas what that's about?

Copy link
Member

@fragglet fragglet left a comment

Choose a reason for hiding this comment

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

Thanks for going to the trouble of putting together a PR. The main thing I'd like to ask about is what the long-term plans are for supporting this. For example: when the next release comes around, are you going to be building and releasing new versions for us too? Is it going to go on the Downloads page of the website?

I think there's definite value in trying to get as much non-Switch stuff merged as possible, but perhaps logistically this might be better off maintained as a separate fork that you can regularly sync from Chocolate Doom HEAD. I'd even be happy with you using the Chocolate Doom name so long as it's just a straight port.

I'm not entirely keen on this kind of construction [to avoid building the setup tool]

Does the setup tool fail to build, or are you just not using it? If the latter, maybe it's not worth the special-casing.

@@ -438,7 +496,12 @@ void I_GetEvent(void)
I_HandleMouseEvent(&sdlevent);
}
break;

// Translate some gamepad events to keys to allow menu navigation
Copy link
Member

Choose a reason for hiding this comment

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

This seems very hacky. I'd rather see the code in m_menu.c extended to support joystick navigation. Indeed, this has already been done somewhat - see #890 and #893 for some prior art.

Copy link
Contributor

Choose a reason for hiding this comment

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

On my mental todo list is port that prior work to heretic,Hexen&strife

Copy link
Contributor

Choose a reason for hiding this comment

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

But I should add @WinterMute I'd be very interested to know what is missing from the existing gamepad-menu support from your perspective

@@ -2201,11 +2203,11 @@ void M_SetConfigDir(const char *dir)
if (strcmp(configdir, "") != 0)
{
printf("Using %s for configuration and saves\n", configdir);
}
// Make the directory if it doesn't already exist:
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mkdir with "" as path crashes on Switch atm. This seems like it might possibly be UB elsewhere too.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think I misread this last night when I was reviewing. I guess it shouldn't ever make sense to mkdir("") anyway, so this should be fine.

@@ -102,6 +102,11 @@ case "$host" in
;;
esac

dnl - platform, for use with game console ports
AC_ARG_WITH([platform],
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this just duplicates what the --host argument does:

https://www.gnu.org/software/autoconf/manual/autoconf-2.65/html_node/Specifying-Target-Triplets.html

I expect devkitpro probably defines a triplet for the Switch that you can use (although I don't know a lot about it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't build Switch specific tools, we're using a standard aarch64-none-elf triplet & using the same tools for a few baremetal (newlib based) targets we're playing with. Adding this option would let us build, for instance, chocolate doom for bare metal Rpi3 if anyone felt like it ;o)

AS_IF([test "x$with_platform" = "xswitch"], [
CFLAGS="-fPIC -march=armv8-a -mtune=cortex-a57 -mtp=soft -ftls-model=local-exec $CFLAGS"
CPPFLAGS="-D__SWITCH__ $CPPFLAGS -isystem /opt/devkitpro/libnx/include"
LIBS="$LIBS -L/opt/devkitpro/libnx/lib -lnx -lm"
Copy link
Member

Choose a reason for hiding this comment

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

Not too crazy about the hard-coded /opt paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're using pacman for the devkitPro toolchains & distributing a bunch of precompiled libraries which kind of necessitates hard coded paths unfortunately. I can use ${DEVKITPRO} here maybe - I'll have a think & see if I can come up with something that seems less hacky.

@@ -3,6 +3,7 @@ EXTRA_DIST= \
README \
doom.ico \
doom8.ico \
doom.jpg \
Copy link
Member

Choose a reason for hiding this comment

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

This file can't be accepted since it contains copyrighted material.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I'll ask around and see if we can get some original artwork for the Switch icons.

Copy link
Member

Choose a reason for hiding this comment

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

Chocolate Doom logo?

This somewhat comes back to my question in the main reply. It's certainly worth upstreaming some of the more general portability fixes, but maybe the Switch stuff belongs more naturally in a fork.


// always enable on Switch
#ifdef __SWITCH__
static int usejoystick = 1;
Copy link
Member

Choose a reason for hiding this comment

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

The cleaner way to do this is to set an override in I_BindJoystickVariables(), eg.

#ifdef __SWITCH__
    usejoystick = 1;
#endif

Or alternatively (probably better) use SDL_GetPlatform() instead.

@WinterMute
Copy link
Contributor Author

The setup tools failed to build when I started but that may actually be alleviated by pulling @LDFLAGS@ out of EXTRA_LIBS & some LDADD settings - part of the issue was getting double -specs arguments at link time.

I think it might be better to run up another PR with the more general fixes & see where we are from there. Would be nice to have the ability to build for Switch upstreamed but I'm hoping the patchset for that will be pretty minimal by the time I'm done anyway.

@fragglet
Copy link
Member

+1 to splitting out the general fixes into a separate PR.

@fabiangreffrath
Copy link
Member

@WinterMute My kids have a Switch. What do I have to do to play Choco Doom on it?

@ericcurtin
Copy link

I'd love to take this fix here:

https://github.com/ericcurtin/opendoom

@ericcurtin
Copy link

Without copyrighted stuff obviously

rfomin pushed a commit to rfomin/chocolate-doom that referenced this pull request Jun 13, 2023
…l-stats-alignment

Fixed Kills line position when switching level stats display from Status bar to a different option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants