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

COMMON: Move the Lua code from SWORD25 to COMMON #1566

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@nipunG314
Copy link
Contributor

commented Mar 31, 2019

This PR moves the Lua code from the SWORD25 engine to COMMON so that Lua can be integrated into other engines.

@bluegr

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thanks for this.

Is there any use for LUA scripts outside of the sword25 engine?

A lot of code has been removed from the LUA interpreter, since it is quite specific to the needs of the sword25 engine. If we need a more generic LUA interpreter, we should adapt the full original LUA code, not this engine-specific interpreter

@nipunG314

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2019

I need the LUA scripts to write the engine for Hyperspace Delivery Boy, so they will probably be used there. If the current LUA setup is not sufficient for this, then I suppose it would make sense to adapt the full original code.

@bluegr

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Well, the original sources have been adapted, but some unused (in sword25) non-portable LUA functionality has been removed, or adapted. An example is the LUA I/O code. Check the history:

https://github.com/scummvm/scummvm/commits/master?after=62fc4c94bfd5b66fbfa444d686c5e86a84bea562+34&path%5B%5D=engines&path%5B%5D=sword25&path%5B%5D=util&path%5B%5D=lua

If you don’t need this functionality, then the interpreter can be used as-is, otherwise, it’ll need to be adapted for this engine. It would be best if you checked the game’s scripts for these cases.

@sev-

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

Thanks.

Now, one more thing is missing. Since now the code lives in the common/, it would be great if you could turn it into an engine feature, similar to png. See sword25/configure.engine

@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

Any news on this?

@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

Any news on this? Now there are conflicts because of changes in PR #1769

@nipunG314 nipunG314 force-pushed the nipunG314:lua-merge branch from 16f0f0b to 18cb4b7 Jul 29, 2019

@nipunG314

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@bluegr Sorry, got seriously sidetracked on this. I have rebased the breaking changes. Will work on the Engine Feature idea next

@bluegr
Copy link
Member

left a comment

Nice work, overall! Only a couple of wording changes left. Well done :)

Show resolved Hide resolved configure Outdated
configure Outdated
@@ -3789,6 +3794,26 @@ case $_host in
;;
esac

#
# Enable Lua only for engines that support it

This comment has been minimized.

Copy link
@bluegr

bluegr Aug 5, 2019

Member

“Enable LUA only for engines that require it”

This comment has been minimized.

Copy link
@Kawa-oneechan

Kawa-oneechan Aug 5, 2019

Contributor

"Lua" is actually correct. It's not an acronym.

This comment has been minimized.

Copy link
@bluegr

bluegr Aug 5, 2019

Member

You are quite correct, my mistake. It's "Lua"

@nipunG314

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

create_msvc wasn't detecting the Lua engine feature. I have re-worked the feature as a default option, and added a Feature Flag for it. create_msvc works properly on it now

configure Outdated
@@ -5397,8 +5405,8 @@ if test "$_build_scalers" = yes ; then
fi
fi

if test "$_mt32emu" = yes ; then
echo_n ", MT-32 emulator"

This comment has been minimized.

Copy link
@bluegr

bluegr Aug 9, 2019

Member

I believe that these have been removed by mistake

This comment has been minimized.

Copy link
@nipunG314

nipunG314 Aug 13, 2019

Author Contributor

Re-added

lua/lua_unpersist.o \
lua/lvm.o \
lua/lzio.o \
lua/scummvm_file.o \

This comment has been minimized.

Copy link
@bluegr

bluegr Aug 9, 2019

Member

These should not be compiled in all the time in common code, which is why the feature flag has been requested. They should be enclosed in a ENABLE_LUA ifdef. i.e:

ifdef ENABLE_LUA
MODULE_OBJS += \
xxx.o
endif

Check common/module.mk for an example

This comment has been minimized.

Copy link
@nipunG314

nipunG314 Aug 13, 2019

Author Contributor

I have wrapped it now

@nipunG314

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

Any other changes required?

@nipunG314 nipunG314 force-pushed the nipunG314:lua-merge branch from 04cb9a7 to 464e2f9 Aug 13, 2019

@nipunG314 nipunG314 force-pushed the nipunG314:lua-merge branch from 464e2f9 to 52df871 Aug 13, 2019

@sev-

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

Great, merging!

@sev- sev- merged commit c1f029c into scummvm:master Aug 13, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.