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

Make the fire terrains actually illuminate their hex #3457

Closed
wants to merge 1,642 commits into from

Conversation

CelticMinstrel
Copy link
Member

Fire illuminates, so it's always weird to see one of these without a corresponding gameplay effect.

jyrkive and others added 30 commits June 26, 2018 20:31
this would bring the whiteboard data of the differnt clients out of sync  which could be a problem since the wb network protocoll identifies the actions to be removed ot replaced by index.
…mption

Fixes wmllint crashing like this upon encountering a gzip tarball in an
add-on:

  Traceback (most recent call last):
    File "/home/shadowm/bin/wmllint-1.14", line 3188, in <module>
      for fn in allcfgfiles(directory):
    File "/home/shadowm/bin/wmllint-1.14", line 2944, in allcfgfiles
      if interesting(os.path.join(root, name)):
    File "/home/shadowm/bin/wmllint-1.14", line 2927, in interesting
      return fn.endswith(".cfg") or is_map(fn) or issave(fn)
    File "/home/shadowm/src/wesnoth-1.14/data/tools/wesnoth/wmltools3.py", line 270, in issave
      return firstline.startswith("label=")
  TypeError: startswith first arg must be bytes or a tuple of bytes, not str

[ci skip]
This is a good point since it ensures any place that might use the title before the lobby
(don't think there's any such places right now) won't inadvertently get formatted.

Also might be worth looking into a way to just strip formatting completely.
…erver

This avoids an issue where people could still apply formatting by using an older client.
Granted, any formatting would still appear in-lobby to anyone using an older client, but
this at least prevents the possibility of formatting appearing indefinitely if a game host
happens to never update their own client.
See pull request #3252. This is the fix @Vultraz prefers to the PR.
This likely existed because this code was added in 2014 before we switched to C++11
and initializer lists.
[ci skip]
Also dropped some unused code in unit_type.

And there's still one in language.cpp, but I'm not entirely sure if it's safe to
drop the last empty entry there so I left it.
There's really no reason to have this anymore. It was (AFAIK) introduced years ago
as a performance-saving measure, then was moved to Advanced Preferences in 1.13 since
modern PCs mostly have no issues. With accelerated rendering, there's even less of a
reason to have this.
[ci skip]

For some reason, this made the entire page stack thinner than it should be. I have
no idea why, and I'm not sure wrap= here instead does anything (namely, wrap if it
gets too long), but it's better than what was happening before.
[ci skip]

Makes it clear it's not intrinsically tied to the Accelerated Speed factor.
This might not be needed anymore now that we no longer have a framebuffer surface.
"Fake interactive" mode seems like it was meant to be an interactive version of fake mode.
However, since we're using it to stop the UI drawing and the unit tests (which use it)
don't seem to display a window at all (not entirely sure why), it's weird that "fake interactive"
mode would be considered interactive.
Also confirmed via PM with beetlenaut that they can be removed, and aren't supposed to still be there.
previously players had to leave an rejoin the game to change their faction.
this was most annoying in coop games where you want might want to choose
your faction in consultation with the other players. Also you previously could
not even checkout the different available factions again after you joined
the game.
in the non-host case the clients might get updates that change the [side] config while the flg dialog is open which might result in invalid pointers,  note that flg_manager::default_leader_cfg_ is still there but this is no problm because that is never dereferenced it's only compared to other pointers.

we also make a copy of all [multiplayer_side] on the non hosts side for the same reason.
hrubymar10 and others added 11 commits August 9, 2018 23:32
code it as &amp; to avoid a pango warning in stderr
3ec26df changed it previously to '+' to work around that warning
also change it for the untranslatable strings in the credits
[ci skip]
This new implementation works more reliably, avoids spawning subshells, and is faster.
This covers the wall torch, the campfire, and the brazier.
@stevecotton
Copy link
Contributor

If sconces illuminate, then Xol (Lit Stone Wall) probably should do too.

Having Xol illuminate means that, if a new player follows the recommended campaign order, scenario 3 of AToTB will be the first scenario when they encounter this. That's possibly a good thing, if #3389 is accepted then AToTB S3 it becomes the main scenario for explaining how local time areas affect the game. If #3389 isn't accepted then I echo sevu's comment in #3056, "A hint in some way would probably not be bad, as [TSG]'s a newbie campaign".

AToTB S3 also has an ^Ebn (Lit Brazer), but that hex isn't likely to see combat, whereas the Xol hexes definitely will.

@stevecotton
Copy link
Contributor

Argh, Xol would be the wall, it's the hexes south, south-east and south-west of the Xol that would need the illumination.

@CelticMinstrel
Copy link
Member Author

I don't think terrain-based illumination like this supports spreading into adjacent hexes, so I guess lit stone wall would need to be handled by a time area instead.

@Vultraz
Copy link
Member

Vultraz commented Aug 21, 2018

Xol is deprecated. If you work on that, it needs to apply to the Sconce terrain. Which this includes. So this might be done.

@jostephd
Copy link
Member

This PR needs to be rebased from old master to new master.

@CelticMinstrel
Copy link
Member Author

Or it could simply be cherry-picked, since it's a single commit.

@GregoryLundberg
Copy link
Contributor

GregoryLundberg commented Oct 20, 2018

  1. I'd cherry-pick it but my mind went "TILT" at over 1600 commits.
  2. This sounds like something which should be classified a bug and backported to 1.14, as well.

@GregoryLundberg GregoryLundberg added the Needs Rebase Pull Requests which are against the wrong branch or are very old and must be brought up-to-date. label Oct 20, 2018
@jostephd
Copy link
Member

@GregoryLundberg curl https://github.com/wesnoth/wesnoth/commit/1f8c7d3fef7d32d204bdb53ca3dc3e82f2e433b2.patch | git am && git push

@GregoryLundberg
Copy link
Contributor

Cherry picked to master and 1.14

@GregoryLundberg GregoryLundberg deleted the illuminating_campfire branch October 20, 2018 20:38
@gfgtdf
Copy link
Contributor

gfgtdf commented Oct 20, 2018

this cannot go into 1.14 since it casues OOS (unit will haev differtn attack strength on clients runniong different versions)

@GregoryLundberg
Copy link
Contributor

OK. Revert it. Or I'll get to it later.

jostephd added a commit that referenced this pull request Oct 20, 2018
@jostephd
Copy link
Member

Reverted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase Pull Requests which are against the wrong branch or are very old and must be brought up-to-date.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet