Code problems

[Yankes]

I looked in OpenXcom code and i have problem with it

`Surface::paletteShift` <- i dont like this function

we have this:

Code: [Select]

https:// store the original palette
_originalColors = (SDL_Color *)malloc(sizeof(SDL_Color) * _surface->format->palette->ncolors);
1) couple of points:
a) why malloc? not beter use `new SDL_Color[_surface->format->palette->ncolors];`?
b) why isnt null assert here? when we call 2 times this function, we will get memory leak.
c) destructor dont delete this memory -> memory leak
d) copy constructor dont copy this

Code: [Select]

if (i * mul + off < _surface->format->palette->ncolors)
{
newColors[i].r = getPalette()[i * mul + off].r;
newColors[i].g = getPalette()[i * mul + off].g;
newColors[i].b = getPalette()[i * mul + off].b;
}
2) is indented that`mul>1` will leave half of colors in palette black?

Code: [Select]

if (_originalColors)
{
SDL_SetColors(_surface, _originalColors, 0, 256);
free(_originalColors);
_originalColors = 0;
}
3) `Surface::paletteRestore()` have fixed size 256, this is indented? in `Surface::paletteShift` the ` _surface->format->palette->ncolors` is used. is possible to have less than 256 in normal palette?

[Daiky]

The malloc I copied that from Palette::loadDat ... not sure why it's used, I guess it's more C-like which fits SDL, maybe it's from an example in the SDL tutorials?

paletteShift is always called once and then paletteRestore after that, so currently there are no memory leaks. But you are right, if someone else wants to use this function it should be made a little more foolproof.

About the "mul", I'm not sure how that works, I copied it from the original function Surface::offset, and it seemed to me it still works?

[Yankes]

The malloc I copied that from Palette::loadDat ... not sure why it's used, I guess it's more C-like which fits SDL, maybe it's from an example in the SDL tutorials?

AFIK is no difference between `malloc` and `new` when its used with C structures. i think is better to stick to C++ functions.

paletteShift is always called once and then paletteRestore after that, so currently there are no memory leaks. But you are right, if someone else wants to use this function it should be made a little more foolproof.

exactly

About the "mul", I'm not sure how that works, I copied it from the original function Surface::offset, and it seemed to me it still works?

probably this function is right, its implementation is bizarre for me.

[Volutar]

"mul" is a ratio. 256 colored VGA mode has 6bit palette (0..63) for each colour. It has to be multiplied by 4 to be ot that dark
So mul apparently equal 4.

[Daiky]

About the "mul"(tiplier):

The font bitmap of x-com has 6 shades of grey = 0 to 5.
(using palette offsets you can blit text in all kinds of colors)
The mul has a max value of 3, because you only can go up to 15 : it sets the contrast higher, as the darker shades get darker.
If you would set it to 4 you end up in the next color range with very weird results.

[Volutar]

I thought it was unadjusted palette issue fixer.

[SupSuper]

Hope that clears everything up.

[Yankes]

now everything is clear

i think we should remove `malloc` here and in `Palette::loadDat`.
i suggest replace pointers with `vector<SDL_Color>` this will remove possibility of memory leak.
only drawback of this approach is ugly syntax when you try get pointer to `SDL_Color`

Code: [Select]

std::vector<SDL_Color> v;
SDL_Color* p = &(v[0]); https:// this one :)
but this can be easy remove by small utility function:

Code: [Select]

template<typename T>
inline T* ptr(std::vector& v, size_t offset = 0) { return &(v[offset]);}
template<typename T>
inline const T* ptr(const std::vector& v, size_t offset = 0) { return &(v[offset]);}
and now we can write this easily

Code: [Select]

SDL_Color* p = ptr(v, 5);
AFIK there will not be any performance differences (except two additional ints that store size and mem_size for vector).

SupSuper if you agree with that change i can implement this.

[SupSuper]

SDL requires a pointer to a SDL_Color array so turning it into a vector would just add unnecessary conversions.
I don't see any issue with replacing malloc with new though.

[Yankes]

SDL requires a pointer to a SDL_Color array so turning it into a vector would just add unnecessary conversions.

after optimization vector will be reduced to plain pointer in case of most operations.
but maybe right, using vector here is bit overkill

[Yankes]

i have new question, what this `const` mean?

Code: [Select]

std::vector<CraftWeapon*> *const Craft::getWeapons()didnt this should be:

Code: [Select]

const std::vector<CraftWeapon*> * Craft::getWeapons()or simply this `const` is useless because function return rvalue and rvalue cant be changed at all (except for C++0x :> )

[SupSuper]

A const has different meanings whether it's to the left or to the right of the *:

- To the left means whatever the pointer is referring to is const (can't be changed). Eg. "const int *a" means "a pointer to a const int", so I can change what the int pointer is pointing to (a = 0), but not the actual pointed int value (*a = 0).

- To the right means the pointer itself is const (can't be changed). Eg. "int *const a" means "a const pointer to an int", so I can change the actual pointed int value (*a = 0), but not what the int pointer is pointing to (a = 0).

In your example, we wanna be able to change the craft's weapons with that function, but not actually change to a new craft weapon vector or something. In practice you're probably right that you couldn't change the original pointer anyways, but um... I saw that convention somewhere?