@lronaldo : done using spaces instead of tabs for indent in asm and example

I'm proceeding now to accept your pullrequest and will review it in its new branch. I will probably make changes and addons to documentation. Please, stay tuned to merge-back new changes to your own repo.

I have added new function cpct_setReplaceColorsM0 to perform colors computation and update other functions to only perform color replacement.

@Arnaud6128 I'm reviewing the code now and doing and in-depth analysis. It's a bad moment to add new stuff to the pull request, even worse if the stuff is changing what I'm reviewing.

It's better if we go step by step. Just let me perform an in-depth review (it will take me time) and then we will be able to discuss future changes to this feature.

In the meantime, it's better if you continue with other features or examples. That will let us paralelize work without collision.

Just a cuestion. Is there any particular reason to design Colorize functions with an output pointer (colorizeSprite)? I mean, does that have any particular advantage? I see that half the functions will be easily improved by just performing Colorize over the same input sprite (modifying it) without having to perform a copy to a new place at the same time.

If you want a copy, you may perform a cpct_memcpy first, then a Colorize, and you would get same results. Unless there is a particular reason for performing Colorize+Copy on the same function you have in mind, I think it would be better to split them out.

@Arnaud6128 I've pushed a suggested version for cpct_spriteColourizeM0_f along with cpct_setSpriteColourizeM0_f. Both of them have the _f postfix from "fast" just to differentiate them. I've also modified your colourReplace example to use and test these two functions.

Please, have a look at the code and interface and let me know what your think. Is this okay for you? Is there any use case or feature missing?

I think this illustrates the concept we've been discussing past days. It also shows how the code gets simplified, it does not require IX or alternate register set, it has no problems if Firmware is on, is faster under same circumstances and can be even faster if you want to do same colour replacement for multiple sprites.

With respect to the conflicts under this branch, it is safer if you undo your commit
04a3424 (just creating a new commit that does reverse changes) and then we can continue working from this point on.

@Arnaud6128 I'm reviewing the code now and doing and in-depth analysis. It's a bad moment to add new stuff to the pull request, even worse if the stuff is changing what I'm reviewing.

• Sorry, i wanted you don't review an old version of my code, it's why i hurriedly pushed my local version i was currently working on. Do you still need i revert my last commit ?

@Arnaud6128 Just a cuestion. Is there any particular reason to design Colorize functions with an output pointer (colorizeSprite)?

• No, i just wanted to keep the original sprite unmodified, but effectively make a cpct_memcpy do the job.

@Arnaud6128 I've pushed a suggested version for cpct_spriteColourizeM0_f along with cpct_setSpriteColourizeM0_f. Both of them have the f postfix from "fast" just to differentiate them. I've also modified your colourReplace example to use and test these two functions.

• I have taken a look to the code and all is ok, i don't see other cases. Do you want to keep no fast functions or they are useless now ?

No problem, man. Sometimes it's complicated to properly coordinate remotely :)

In this particular case, I think it will be better keeping just the _f version, as there is no propper advantage for the previous version. Same functionality to the previous version can easily be achieved with a macro that takes all the parameters and performs the 3 calls.

I haven't had a look to M1 functions yet. If you want, yo may try to apply same idea to equivalent M1 function and see how it goes.

From now on, its better if we go one function at a time and take time to consider them thoroughly. It will take us time, but we will deliver the best quality possible, as always :)

Revert done.

OK, if i understand well, from now i'll apply same modifications of cpct_setSpriteColourizeM0_f and cpct_spriteColourizeM0_f for Mode 1 and you'll continue to take a look on others colourize M0 functions.
That's it ?

Yes, while you apply changes to M1 functions and see how it goes, I will continue reviewing the other functions and completing documentation.

Add cpct_setSpriteColourizeM1 and update cpct_spriteColourizeM1

Merged into lronaldo:colorreplace branch, reviewed and improved a little bit. Nicely done, @Arnaud6128 👍

Please, check your editor under Windows: it keeps changing unix line-endings into ms-dos line endings, then ^M caracters appear on line-endings. Configure your editor to use unix line-endings to prevent this from happening.

• Update cpct_spriteMaskedColourizeM1
• Update cpct_setSpriteColourizeM1 with new placeholders cpct_spriteMaskedColourizeM1_pxX.
  I have no found how to reuse existing placeholders cpct_spriteColourizeM1_pxX in function cpct_spriteMaskedColourizeM1 without compilation errors

@Arnaud6128 using same set function both for cpct_spriteColourizeM1 and cpct_spriteMaskedColourizeM1 is not a good idea. It is much modular and better to have 2 separate set functions, one for each Colourize version. The reason is twofold:

• cpct_setSpriteColourizeM1 becomes bigger and slower because it has to set placeholders for both functions.
• A user wanting only one of the two functions will have both of them included in the final binary. As cpct_setSpriteColourizeM1 points to, and changes contents from both functions, if the user calls cpct_setSpriteColourizeM1 the linker will automatically include cpct_spriteColourizeM1 and cpct_spriteMaskedColourizeM1 even if you are using only one of them in your code.

As this is clearly undesired behaviour, it is much preferable to have one set function for each Colourize function.

Ok i'll do that.

Is cpct_setSpriteMaskedColourizeM0 is to be created or it is simply missing in repo ?

@Arnaud6128 Please, merge and check my latest commits. I've reviewed your new functions and added cpct_setSpriteMaskedColourizeM1. Have a look at documentation reviews for improvements.

M0 version is still to be created, yet I have included it on the header file.

As always, nice work man 😄

Added cpct_setSpriteMaskedColourizeM0 and update example to use it

When i compile the example i have multiple definition error :

?ASlink-Warning-Definition of public symbol 'cpct_spriteColourizeM0_px0_oldval' found more than once:
   Library: '/home/ArnaudPC/cpctelera-1.5.0/cpctelera/cpctelera.lib', Module: 'cpct_spriteColourizeM0_asmbindings.rel'
   Library: '/home/ArnaudPC/cpctelera-1.5.0/cpctelera/cpctelera.lib', Module: 'cpct_spriteColourizeM0_cbindings.rel'
Multiple definition of cpct_spriteColourizeM0_px0_oldval
Multiple definition of cpct_spriteColourizeM0_px1_oldval
Multiple definition of cpct_spriteColourizeM0_px0_newval
Multiple definition of cpct_spriteColourizeM0_px1_newval
make: *** [/home/ArnaudPC/cpctelera-1.5.0/cpctelera//cfg/global_main_makefile.mk:50: obj/color.ihx] Error 1

@Arnaud6128 I've checked your branch to a local one and have had no problem compiling it. It might be due to a bad incremental compilation of the library. Please, try make cleanall && make next time you compile cpctelera.lib and see if that fixes the issue.