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.

Sign up for GitHub

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

Jump to bottom

Special resource and TS chem missile #20408

Open
wants to merge 1 commit into
base: bleed
Choose a base branch
from
Open

Special resource and TS chem missile #20408

wants to merge 1 commit into from

Conversation

dnqbob
Copy link
Contributor

@dnqbob dnqbob commented Oct 29, 2022

Special resource can charge super weapon.

@PunkPun
Copy link
Member

PunkPun commented Oct 29, 2022

After this release we are planning on a major resource refactor, one that would allow addition of veins as a resource without introducing any new code. This PR will likely not be merged until then

@dnqbob dnqbob marked this pull request as draft November 19, 2022 11:59
Mailaender
Mailaender previously approved these changes Nov 19, 2022
View reviewed changes
Copy link
Member

@Mailaender Mailaender left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as promised.

@dnqbob
Copy link
Contributor Author

dnqbob commented Nov 19, 2022

Should I wait for "major resource refactor" before modifying? Maybe at that time the change here will need to open a new PR

@Mailaender
Copy link
Member

I think we can take this as a temporary solution.

@PunkPun
Copy link
Member

PunkPun commented Nov 19, 2022

It's most likely that this PR will be completely redundant as veins will be programable just with yaml. No need for extra hardcoding

@dnqbob
Copy link
Contributor Author

dnqbob commented Nov 19, 2022 •
edited

OK, but I cannot fix those now because I need to debug some desync issue, mark it draft for now

@dnqbob dnqbob force-pushed the tox-mils branch 2 times, most recently from 6aaecd5 to f4ac9f7 Compare November 28, 2022 12:42
@dnqbob dnqbob marked this pull request as ready for review November 28, 2022 12:42
@dnqbob
Copy link
Contributor Author

dnqbob commented Nov 28, 2022

Sorry for the delay for more than a week, I took many time on debug #20478, and also found the #20475 and #20468 during this time.

@Mailaender
Copy link
Member

You made it sound like this would add a desync.

@dnqbob
Copy link
Contributor Author

dnqbob commented Dec 4, 2022 •
edited

Nope, although I found #20478 in my repo and once suspected this commit was the fault of #20478.

Is there anyway to cancel its link to other issue/pr?

abcdefg30
abcdefg30 reviewed Jan 12, 2023
View reviewed changes
OpenRA.Mods.Common/Traits/StoresSpecialResources.cs Outdated
@@ -0,0 +1,86 @@
#region Copyright & License Information
/*
* Copyright 2007-2022 The OpenRA Developers (see AUTHORS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be adjusted for #20595.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be fixed

@dnqbob dnqbob force-pushed the tox-mils branch 2 times, most recently from 8de46f6 to 74f1600 Compare January 24, 2023 08:45
@penev92
Copy link
Member

penev92 commented Jan 24, 2023

OpenRA.Mods.Common/Traits/Player/PlayerResources.cs(109,4): error IDE0007: use 'var' instead of explicit type

@penev92
Copy link
Member

penev92 commented Jan 24, 2023

Sorry, CI is failing again.

OpenRA.YamlException: Sequence cloud.cloud1 does not define a filename.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mailaender Mailaender Mailaender left review comments

@abcdefg30 abcdefg30 abcdefg30 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
PR: Conceptual concerns PR: Needs +2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dnqbob @PunkPun @Mailaender @penev92 @abcdefg30 @EoralMilk