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

Add support for traps + spike traps + fixed several crash + console command for listing animations from an ogre mesh #209

Merged
merged 7 commits into from Oct 1, 2014

Conversation

hwoarangmy
Copy link
Contributor

To test the traps, it's pretty easy. Launch the 2v2 skirmish test map. I've added 2 spike traps near your dungeon. See what happens when your creatures go there ^^
I've also added a claimed tile on the bottom right player to test the traps

…oomobjects are destroyed

fixes #139
fixes #200

- Changed Creature::doTurn for Creature::doUpKeep
- The ennemy/allied visible objects are filled by the tile instead of gamemap
- "You have won!" should not be a standard chat message. It should be a server chat message
@hwoarangmy
Copy link
Contributor Author

Do you want to merge as it is or should I do the requested changes in this PR ?

@Bertram25
Copy link
Contributor

@hwoarangmy Sorry for not answering earlier, I had to run somewhere else (RL stuff).
I'll answer all the other questions asap, but from what I've seen, you can do that in another PR if it's better for your own organization.

regards,

@hwoarangmy
Copy link
Contributor Author

I've added the requested changes after all ^^

@Bertram25
Copy link
Contributor

The code looks fine. I'll test asap (more or less in two hours). Thanks for all the fixes :)

@oyvindln
Copy link
Contributor

oyvindln commented Oct 1, 2014

Since we are using C++11, we should really be using "nullptr" rather than "NULL".

@akien-mga
Copy link
Member

The trap works great :-D I even managed to claim one of the two enemy traps in the player base in the skirmish 2v2 test map. For the second one, I need more Chuck Norris mechanical diggers :-D

@hwoarangmy
Copy link
Contributor Author

Since we are using C++11, we should really be using "nullptr" rather than "NULL".

I didn't know it was for c++11. I will use it then ^^

@Bertram25
Copy link
Contributor

Since we are using C++11, we should really be using "nullptr" rather than "NULL".

@oyvindln Just out of curiosity, what made the team decide to use c++1x in the first place?
As for the use of nullptr, I'll be honest, that won't prevent me from merging PRs. ;) Even if you're right about it.

@Bertram25
Copy link
Contributor

Works like a charm, for both trap types :) Editor fixes for rooms and light confirmed.
Can't confirm for the random crashes but I trust you upon those.

Merging...

Bertram25 added a commit that referenced this pull request Oct 1, 2014
Add support for traps + spike traps + fixed several crash + console command for listing animations from an ogre mesh
@Bertram25 Bertram25 merged commit a295048 into OpenDungeons:development Oct 1, 2014
@oyvindln
Copy link
Contributor

oyvindln commented Oct 1, 2014

I can't remember why we enabled C++11, though it's the standard, and contains a lot of useful stuff.

@Bertram25
Copy link
Contributor

... and a lot of less useful ones. And just to be clear it's a standard, not the standard. Hence my resilience about certain new keywords that adds nothing, for instance, since they aren't used by oldies and misused by newbies. Old bear speaking. ;)

That said, since it's the chosen standard by the project, yes nullptr should be used.

@akien-mga
Copy link
Member

I don't have any experience with C++, but I think it's never a bad move to switch to a newer standard. I'm a bit sad to see so many great projects are still using Python 2 when Python 3 has been around for 6 years now. I understand that projects that started at the time of Python 2 could be reluctant to port all their codebase, but new projects should definitely use the newest implementation. I guess that applies (at least partly) to C++ too?

@Bertram25
Copy link
Contributor

I guess that applies (at least partly) to C++ too?

Ah sure. Don't misunderstand me. :) @oyvindln is completely right and we should move on with that. Plus, if we never use it, we'll never actually know what's good and bad. ;) It was just was my human reluctancy to change expressing itself. ;) Now that's done, I'm feeling lighter. :P

@Bertram25
Copy link
Contributor

Just to be clear, is that we should use it but not enforce it and use it as an excuse to prevent a merge. IMHO, if the code is proper c++, it should go in. If there is use of the newer standard in a good way, then all the same. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants