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

Add preprocessor #commands to pause and resume preprocessing. #2078

Open
ProditorMagnus opened this Issue Oct 6, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@ProditorMagnus
Contributor

ProditorMagnus commented Oct 6, 2017

Currently, there is << >> syntax, which works for value of single key, but needs additional work to protect longer code.

Use case would be releasing already preprocessed addon, in order to reduce loading time for players.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 6, 2017

Member

That would be a single-file (at least WML-wise) add-on then? The preprocessor also handles file inclusions.

Member

AI0867 commented Oct 6, 2017

That would be a single-file (at least WML-wise) add-on then? The preprocessor also handles file inclusions.

@ProditorMagnus

This comment has been minimized.

Show comment
Hide comment
@ProditorMagnus

ProditorMagnus Oct 6, 2017

Contributor

Yes. I have implemented it as single file of addon wml, plus _main.cfg which loads it if preprocessed version exists (made to valid unprocessed wml with some regex).

Contributor

ProditorMagnus commented Oct 6, 2017

Yes. I have implemented it as single file of addon wml, plus _main.cfg which loads it if preprocessed version exists (made to valid unprocessed wml with some regex).

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 6, 2017

Member

Hmm, I see two main options here, either some sort of pragma to disable preprocessing for (the rest of) the file, or a different extension for already-preprocessed WML (C and C++ compilers can do this, for similar reasons). We could add a switch to wesnoth to generate such a file.

A different question though, is how much more efficient this is. Have you profiled or tested things?

Member

AI0867 commented Oct 6, 2017

Hmm, I see two main options here, either some sort of pragma to disable preprocessing for (the rest of) the file, or a different extension for already-preprocessed WML (C and C++ compilers can do this, for similar reasons). We could add a switch to wesnoth to generate such a file.

A different question though, is how much more efficient this is. Have you profiled or tested things?

@ProditorMagnus

This comment has been minimized.

Show comment
Hide comment
@ProditorMagnus

ProditorMagnus Oct 6, 2017

Contributor

With SSD there was no notable difference, but on HDD it saves about a minute.

https://forums.wesnoth.org/viewtopic.php?p=616920#p616920

https://www.wesnoth.org/irclogs/2017/06/%23wesnoth-dev.2017-06-03.log starting with

20170603 13:28:46< Ravana_> 20170602 11:14:57< Ravana_> I wonder if there would be notable performance boost if I uploaded preprocessed addon, instead of development version with macros
20170603 13:28:48< Ravana_> I did some testing of single-file addon vs normal folder structure. Results on HDD were: normal loading time: [1:22, 1:08, 1:34], singlefile loading time: [0:26, 0:29, 0:19, 0:19]. On SSD both versions loaded too fast to consider results useful.

Contributor

ProditorMagnus commented Oct 6, 2017

With SSD there was no notable difference, but on HDD it saves about a minute.

https://forums.wesnoth.org/viewtopic.php?p=616920#p616920

https://www.wesnoth.org/irclogs/2017/06/%23wesnoth-dev.2017-06-03.log starting with

20170603 13:28:46< Ravana_> 20170602 11:14:57< Ravana_> I wonder if there would be notable performance boost if I uploaded preprocessed addon, instead of development version with macros
20170603 13:28:48< Ravana_> I did some testing of single-file addon vs normal folder structure. Results on HDD were: normal loading time: [1:22, 1:08, 1:34], singlefile loading time: [0:26, 0:29, 0:19, 0:19]. On SSD both versions loaded too fast to consider results useful.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 6, 2017

Member

Actually, preprocessing is something that can already be done. There is a --preprocess option which outputs a regular cfg file with only #textdomain directives remaining. (which are required to make translations function)

Of course, the preprocessor will still run on this file, but that's not really a problem, is it?

Member

AI0867 commented Oct 6, 2017

Actually, preprocessing is something that can already be done. There is a --preprocess option which outputs a regular cfg file with only #textdomain directives remaining. (which are required to make translations function)

Of course, the preprocessor will still run on this file, but that's not really a problem, is it?

@ProditorMagnus

This comment has been minimized.

Show comment
Hide comment
@ProditorMagnus

ProditorMagnus Oct 6, 2017

Contributor

Yes, I generate that file with wesnoth -p. So far I have noticed that it breaks some units.w.o faction names, resulting in name like "AE - #textdomain wesnoth-Ageless_Era".

Problem of running preprocessor again is that the generated cfg file is not immediately valid - at least if Lua is used. It drops <<>>, so they need to be swapped in again.

Contributor

ProditorMagnus commented Oct 6, 2017

Yes, I generate that file with wesnoth -p. So far I have noticed that it breaks some units.w.o faction names, resulting in name like "AE - #textdomain wesnoth-Ageless_Era".

Problem of running preprocessor again is that the generated cfg file is not immediately valid - at least if Lua is used. It drops <<>>, so they need to be swapped in again.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 6, 2017

Member

I suppose the values are because of stuff like this?

 #textdomain wesnoth-units
         description=_"Blood Bats are so named because of their ruddy hue, which some mark as a symbol of their preferred diet. These creatures are fast and can drain the blood of those they attack, thereby gaining some of the health lost by their victims." +
 #textdomain wesnoth-help
             _"

 Special Notes:" +
             _" During battle, this unit can drain life from victims to renew its own health."

As for the lua stuff, I suppose we need to check if things need to be escaped before printing, and use different quotation based on that.

Member

AI0867 commented Oct 6, 2017

I suppose the values are because of stuff like this?

 #textdomain wesnoth-units
         description=_"Blood Bats are so named because of their ruddy hue, which some mark as a symbol of their preferred diet. These creatures are fast and can drain the blood of those they attack, thereby gaining some of the health lost by their victims." +
 #textdomain wesnoth-help
             _"

 Special Notes:" +
             _" During battle, this unit can drain life from victims to renew its own health."

As for the lua stuff, I suppose we need to check if things need to be escaped before printing, and use different quotation based on that.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Oct 6, 2017

Contributor

I'm unsure why you need a preprocessed addon in the first place. isn't that exactly why we have our wml cache? That is to make the proprocessign only happen once and the read from the cached prorocerssed file

Contributor

gfgtdf commented Oct 6, 2017

I'm unsure why you need a preprocessed addon in the first place. isn't that exactly why we have our wml cache? That is to make the proprocessign only happen once and the read from the cached prorocerssed file

@ProditorMagnus

This comment has been minimized.

Show comment
Hide comment
@ProditorMagnus

ProditorMagnus Oct 6, 2017

Contributor

The WML cache is only used until wesnoth exits, then it is regenerated. Might be different for 1.13 though.

Contributor

ProditorMagnus commented Oct 6, 2017

The WML cache is only used until wesnoth exits, then it is regenerated. Might be different for 1.13 though.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 6, 2017

Member

Actually, it's stored in a cache directory and will be reloaded from there unless files have changed.

Member

AI0867 commented Oct 6, 2017

Actually, it's stored in a cache directory and will be reloaded from there unless files have changed.

@ProditorMagnus

This comment has been minimized.

Show comment
Hide comment
@ProditorMagnus

ProditorMagnus Oct 6, 2017

Contributor

I see that with v1.13.9+dev (3786827-Modified) it indeed makes use of that. That is enough for my case.

Nevertheless, the requested feature might be useful for someone.

Contributor

ProditorMagnus commented Oct 6, 2017

I see that with v1.13.9+dev (3786827-Modified) it indeed makes use of that. That is enough for my case.

Nevertheless, the requested feature might be useful for someone.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Oct 7, 2017

Contributor

That said, i think the wml cache shudl be improved

  1. It currently reloads all addons if a macro paramter (like campaigm_define, of difficulcy) has changed even if a addon doesn't use those ? For example If you play the LoW mp campaign it will reload the whole ageless stuff, since the macroparamters (DIFFICULCY and CAMPAIGN_LOW) have changed, it'd be better if the proprocecor would eigher keep track of which macros are actually used, or if the addons wml woudl be allowed to explcitly list all macros it depends on.
  2. It currently only checkes whether files in the current addon have changed, so if you include a file from another addon, the wml cache might create wrong wml.
Contributor

gfgtdf commented Oct 7, 2017

That said, i think the wml cache shudl be improved

  1. It currently reloads all addons if a macro paramter (like campaigm_define, of difficulcy) has changed even if a addon doesn't use those ? For example If you play the LoW mp campaign it will reload the whole ageless stuff, since the macroparamters (DIFFICULCY and CAMPAIGN_LOW) have changed, it'd be better if the proprocecor would eigher keep track of which macros are actually used, or if the addons wml woudl be allowed to explcitly list all macros it depends on.
  2. It currently only checkes whether files in the current addon have changed, so if you include a file from another addon, the wml cache might create wrong wml.
@ProditorMagnus

This comment has been minimized.

Show comment
Hide comment
@ProditorMagnus

ProditorMagnus Oct 15, 2017

Contributor

However, considering wmlunits timeout, I still need to create preprocessed version for 1.13 too - that has timeout of 20sec.

Contributor

ProditorMagnus commented Oct 15, 2017

However, considering wmlunits timeout, I still need to create preprocessed version for 1.13 too - that has timeout of 20sec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment