Refactorings

International board Development
#41

Funny fact: it was you who replaced vector with vector when I used it for FoW at the beginning of time. That’s how I learnt that vector is evil.

#42

In rev 3154 i used vector to store bitmask - it is exactly wherefore vector is intended.

#43

Game crashes for me as soon as bit vector is serialized.

Can’t we just use bitset for same purpose?

#44

“Bitsets have a fixed size. For a similar container class that also optimizes for space allocation and allows for dynamic resizing, see the bool specialization of vector (vector).”

EDIT
So the easiest way is reverting to vector

#45

Well, yes.
Though still optimization that it uses rarely is desirable and it is unpleasant to generically process. We care much more about accessing elements time than saving memory.
Still, given the scale of h3m bitmaps this is not an issue.
I like the better expressiveness of vector. Bool should be bool.
For the vectors of allowed things (like spells) I’d also might consider just vector.

Is this a vector issue?
I have crashes at map loading, it may be a bug elsewhere.
(Investigation in progress…)

#46

Then set or set IMO better.

#47

But then set is typically implemented as RB tree and has overhead of 3 pointers per stored item. vector saves too much memory, set wastes it too much. :stuck_out_tongue:
… I too much think about premature optimizations. Ignore it. First care about expressive semantics of the code. The lists are not that long, use whatever you want.

I found vector issue, it’ll be fixed in a few mins.

EDIT: Fixed. :slight_smile:

#48

I think more about unification than about performance right now.
F.E. hero spells is set now, allowedXXX are vectors.

But priority is hunting code duplication, so I`ll not change containers unless there will be more issues with them.

#49

@AVS:

There are a few things to mention regarding your latest refactorings in the map loading code.

  1. Source files can be named relatively freely, but to be consistent with the rest of our code base the extracted H3M loader class file should be named different. You’ve named it MapFormatH3M.cpp/.h, better would be CMapLoaderH3M.cpp/h. It is already in SVN which is bad, but perhaps you could update it.

  2. I’m not really happy that you’ve added all those readXXX helper methods. They don’t belong into the h3 map loader class. Why haven’t you used the CBinaryReader, this should be exactly what you need. Ask me any questions if sth. is unclear. So we have code duplication, inline code in the header file,…

bool areSpells = buffer[pos]; 
++pos; 
	 	 
if(areSpells) 
{
...
}

This is the former code.

if(readBool())
{
...
}

This is how it looks refactored. Is this really better? Now you don’t know what’s done inside the if and you have to add a comment. This isn’t the only place, where you’ve refactored like this example.

Moreover the unique_ptr around CMapHeader in the loader class can be removed. The ownership is clear, using it here is like taking a sledgehammer to crack a nut. I’ve over-used it here a little bit. (and at some other places too). Thanks.

Don’t take my words too seriously, I just had to get that off my chest:)

#50
  1. file name is selected taking in mind future saving support. (for future editor and RMG)
  2. Binary reader is next step
  3. OK will be changed during 2)
#51

Ok, that’s fine!

#52

AVS, as to spell handling… since you started refactorings, hardly any spell works correctly. It’s especially visible for damage spells which seem to work at random. I noticed issue with Summon Elementals, but players also reported Magic Mirror, Dispel, Animate Dead and probably others I don’t remember at the moment. All in all, spells are barely usable in 0.91 and it’s not easy to track all the changes.
So please check if changes you commit don’t actually break the spells or at least mention if they can do.

#53

Are there any reproducible bugs?

UPD: May be it`s time to think about some automatic regression testing?

#54

Also, for map editor, can you remake hidden features of map editor from H3 SOD, so we can make maps(or campaigns) in NWC style.

#55

I’ve just started the editor project. I won’t push it far, just want it to be there. It uses Qt 5 mainly for its new connect syntax.

zmudziak22, the most important thing about this editor is that I’d like to support old formats (SoD, WoG, maybe even HotA). I hope it will be the best map editor for Heroes III (not only VCMI) ever.

#56

We have “extern SDL_Surface* screen;” declared in many .cpp files across the project instead of being defined once in some global header which is obviously a very bad practice. What would be the best header for “extern SDL_Surface* screen;” to be declared once?

#57
  1. OpenGL rendering branch is being developed
  2. it’s not a bug actually
  3. I doubt there is a good place for it now
    so I think we should leave it and wait for OpenGL.